-
-
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?
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 originally did this, but I pivoted as creating the URLs is not free CPU wise. We have a huge manifest and creating 100's of URLs in startup isn't ideal. I believe it's better perf wise to create on demand. We could cache in future if need be though but I just kept it simple.
if (resolved == null) { | ||
throw new Error('Could not resolve bundle with id ' + id); | ||
} | ||
return resolved; | ||
return new URL(resolved.path, resolved.baseUrl).toString(); |
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.
This one definitely won't work in IE. Probably ok at this point? I guess we can't simply concatenate the strings? path
could probably start with ../
or something...
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.
Yeah not ideal. We could just recommend a polyfill for URL if anyone has issues I guess.
↪️ 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