-
-
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
Fix new ESM loader runtime by moving to abs URLs #9172
Conversation
@@ -1,18 +1,17 @@ | |||
var mapping = {}; | |||
var mapping = new Map(); |
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.
Are we ok to drop browsers that don't support Map
? I think Parcel's output still worked in IE11 up until this point. Maybe it's ok, but wonder if it should be done in a major version?
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 believe Map is supported in IE11. This support coverage looks good enough to me. But I can remove if you want.
https://caniuse.com/mdn-javascript_builtins_map
Co-authored-by: Marcin Szczepanski <mszczepanski@atlassian.com>
Co-authored-by: Marcin Szczepanski <mszczepanski@atlassian.com>
…to esm-runtime-fix
let baseUrl = | ||
entryBundle.env.outputFormat === 'esmodule' && | ||
entryBundle.env.supports('import-meta-url') | ||
? 'new __parcel__URL__("").toString()' // <-- this isn't ideal. We should use `import.meta.url` directly but it gets replaced currently |
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.
@devongovett Thoughts on this? Seems reasonable that I should be able to use import.meta.url
without it being replaced?
I could modify the transform to add a new special parcel symbol (e.g. __parcel_importMetaUrl__
)? Or potentially add some asset meta data that prevents the stripping?
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.
hmm yeah, I guess we could add a new __parcel
thing, but does this work as is for now? if so I'm ok with leaving it as you have it as well.
Benchmarks are failing in the size comparison code (https://github.com/parcel-bundler/parcel-benchmark-action/blob/master/src/utils/compare-benchmarks.ts#L37-L65) - my guess is this is because it expects all the bundles produced in the baseline to also be present in the target, and you've renamed some of the runtime bundles. |
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 ok. couple questions but I wouldn't consider them blocking
mapping.set(manifest[i], { | ||
baseUrl: baseUrl, | ||
path: manifest[i + 1], | ||
}); |
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 could do the new URL
thing here instead of storing both and doing it every time we resolve?
↪️ Pull Request
This PR fixes the ESM loader runtime by making the bundle-manifest return absolute URLs. It also makes the following changes:
🚨 Test instructions
✔️ PR Todo