-
Notifications
You must be signed in to change notification settings - Fork 2
CLOUDP-352308 Developer experience improvements and webpack HMR support #2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
CLOUDP-352308 Developer experience improvements and webpack HMR support #2
Conversation
conf: ignore package-lock.json
sometimes browser recognizes these js files as "xml" for some reason
feat: use checkbox for compressed and wds checkbox
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're hitting the ground running, this is great! Just had a few thoughts. I'm also not 100% sold on the whole PlaceholderRegistry approach, but I'm coming around on the idea the more I look at it.
| <main> | ||
| <label><span>Match URL prefix</span><input name="match"></label> | ||
| <label><span>Replace URL prefix</span><input name="replace"></label> | ||
| <label><span>Webpack Dev Server?</span><input name="wds"></label> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In retrospect, it's ridiculous that we left this as a text input for as long as we did
options.html
Outdated
| <span>MongoCDN Presets:</span> | ||
| <input type="button" name="preset" value="Development"/> | ||
| <input type="button" name="preset" value="QA"/> | ||
| <input type="button" name="preset" value="Production"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the initial idea for the plugin when Justin Moses made it way back in... 2017 or whatever... was that it'd be more general-use, but since Eric moved it into mongodb-labs... having presets is a fantastic idea!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking a bit more about your comment Jeremy, what do you think of simplifying config even further? Like:
- removing the configurable source base-url and only having the presets.
- removing the compressed and wds options and only letting users specify their webpack-dev-server local assets url.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The real question to me, the more I think about this PR, is whether we want to continue supporting Redwood at all, or if we just want to maintain config that can be imported into that tool you demonstrated at Web Chapter (which I've already forgotten the name of). It seems like that tool is already a clear massive step up for the more "generic" approach, and might also be a simpler DX in general, since it'd avoid the kind of hacky installation process we have with this tool right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately there are not many browser extensions that do rule-based http(s) interception, I found these two in my search:
- https://github.com/vvmgev/Inssman It's a bit janky, export/import has small issue, http-response interception has small issue, a maintainer removed regex matching support for no good reason recently.. that is what I found out in a day there should be more.
- https://github.com/requestly/requestly Although open-source it's commercial and only allows defining 4 rules or so.
Me personally I'm pretty happy with using https://www.mitmproxy.org/ and writing interception rules. I've developed one for myself, that redirects assets to webpack-dev-server, overrides feature-flags, forces delay to test loading shimmers. It's easier to test/develop/maintain in general than a browser extension.
options.html
Outdated
| <input type="button" name="preset" value="Production"/> | ||
| </div> | ||
| <br/> | ||
| <label for="match"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do <label> elements actually need a for attribute when they're wrapping the input?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, removed it
service_worker.js
Outdated
| // Uncomment for seeing matches (not a lot of info). | ||
| chrome.declarativeNetRequest.onRuleMatchedDebug.addListener((info) => { | ||
| console.log(JSON.stringify(info)); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we'd had this commented out before, should we leave that as the better default? Or if not, we should at least update the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better default, I like that, the performance penalty should be negligible too.
| } | ||
| const cssAssetRegexSub = `${replace}/static/assets/css/\\1${addMin('css')}.css` | ||
|
|
||
| const newRules = createRules(config); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well now I have Dua Lipa stuck in my head
rules.js
Outdated
| const isWDS = wds === 'true'; | ||
| const isCompressed = compressed === 'true'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still accurate? It looked like you'd made some changes to store booleans instead of strings, is Chrome translating that internally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if there's any merit in keeping support of non webpack-dev-server redirects?
what do you think of removing all these extra configs π€
rules.js
Outdated
| // JS chunks | ||
| createRedirectRule( | ||
| '[orig_url]/static/[segment]/[name].[hash].js', | ||
| '[repl_url]/static/[segment]/[name][min].js', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little bit skeptical of using a custom syntax parser tool, but I can't deny that this is really nice and readable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very valid point, I have been thinking about this comment for days.
I think named capture regex groups is a great alternative, one caveat though that chrome dynamic rules don't support it.
I can write a small helper function to replace named capture regex groups to numbered ones. That way we are not inventing a brand new syntax here, and just using a known regex feature (but feeding it to chrome rule engine with a trick)

π Description
Firstly, thank you so much for this useful and nice chrome extension! I had some issue with react HMR so I decided to contribute.
β¨ New Features
.hot-update.*)ποΈ Technical Improvements
Create reusable regex utility with webpack-style patterns ([segment],[hash],[name], etc.) to define redirect rules in a more readable way.Rewrite redirect rules using webpack-style patterns in a separaterules.jsmodule with decent test coverage.π οΈ Configuration Improvements
.prettierrcfor consistent code formattingpackage.jsonto pull in type hints for Chrome APIs and Node.js.tool-versionsto ensure unit tests pass..vscode,.idea) andpackage-lock.json.π§ͺ Testing