-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Add manifest V3 true HMR #8145
Add manifest V3 true HMR #8145
Conversation
Benchmark ResultsKitchen Sink 🚨
Timings
Cold BundlesNo bundles found, this is probably a failed build... Cached BundlesNo bundles found, this is probably a failed build... React HackerNews ✅
Timings
Cold BundlesNo bundle changes detected. Cached BundlesNo bundle changes detected. AtlasKit Editor ✅
Timings
Cold BundlesNo bundle changes detected. Cached BundlesNo bundle changes detected. Three.js ✅
Timings
Cold BundlesNo bundle changes detected. Cached BundlesNo bundle changes detected. |
This is very imperfect but fixes a major bug with |
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.
Thanks, this looks good. A few questions.
try { | ||
__parcel__importScripts__(asset.url); | ||
resolve(); | ||
} catch (err) { |
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.
importScripts
is synchronous, so this would currently catch any type error thrown by the imported code too. Maybe better to detect ESM (e.g. try..catch around import.meta
, which throws an error when not in a module).
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 thought import.meta
was invalid syntax in some browsers but I can change it.
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.
Going to send the outputFormat from the HMR server as part of the asset instead.
? window | ||
: typeof global !== 'undefined' | ||
? global | ||
: {}; |
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.
Why was this necessary? global
should be injected by parcel when needed...
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.
It wasn't for some reason. I don't think global
is actually a property that exists in browsers anyway...just in Node.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.
Parcel should be polyfilling it https://parceljs.org/features/node-emulation/#shimming-builtin-node-globals
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 tested and it seems to be working, but maybe I missed something
Pushed a few small updates. Hopefully I didn't break anything. Let me know. |
↪️ Pull Request
Adds true HMR for all assets in a Manifest V3 web extension. Unfortunately this is mostly useless for the foreseeable future because of a Chromium bug. However hot reload at least works for the options page and sandbox pages now, just not for content scripts.
Fixes #8168, which currently breaks pretty much all MV3 extensions with the default background script.
✔️ PR Todo