-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Support import.meta.url #2164
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
Support import.meta.url #2164
Conversation
c049780 to
2a6b9e0
Compare
|
@guybedford Does this transform preserve the URL of the original module, rather than return the URL of the bundle? |
lukastaegert
left a 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.
Looks good, just some minor points
- MetaProperty should be a class
- Maybe there should be more alignment between the UMD and AMD cases
src/ast/nodes/MetaProperty.ts
Outdated
| return node.type === NodeType.MetaProperty; | ||
| } | ||
|
|
||
| export default interface MetaProperty extends Node { |
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 should probably be a real AST node class instead of an interface as otherwise, it will never be tree-shaken (as internally, it will be an UnknownNode).
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.
Sure. Does this mean that MetaProperty nodes haven't been treeshakable until now though?
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.
Yes, that's what it means. Which is something they share with spread elements. Though the latter are very hard to get right due to the complex hidden mechanics.
src/ast/nodes/MemberExpression.ts
Outdated
| storeName: true, | ||
| contentOnly: true | ||
| }); | ||
| } else if (isMetaProperty(this.object) && this.object.meta.name === 'import') { |
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.
If MetaProperty is a class, this can become an instanceof check
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.
Another advantage would be that we could move the meta logic to the meta class and thus keep the member expression a little cleaner (i.e. even though the overwriting needs to happen in the member expression, the code that is used for overwriting could be taken from the meta property or from an import in the same file).
| umd: undefined, | ||
| iife: undefined | ||
| }; | ||
|
|
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 moving it here makes a lot more sense 👍
src/ast/nodes/MemberExpression.ts
Outdated
| cjs: `new (typeof URL !== 'undefined' ? URL : require('url').URL)('file:' + __filename).href`, | ||
| iife: globalImportMetaUrlMechanism, | ||
| umd: globalImportMetaUrlMechanism | ||
| }; |
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.
My assumption was that it should be possible to switch an AMD module for a UMD modules without changed functionality which does not seem to be the case here. Maybe it is possible to make these two cases more similar in case an AMD loader is used?
src/ast/nodes/NodeType.ts
Outdated
| export const VariableDeclarator = 'VariableDeclarator'; | ||
| export const VariableDeclaration = 'VariableDeclaration'; | ||
| export const WhileStatement = 'WhileStatement'; | ||
| export const YieldExpression = 'YieldExpression'; |
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 only thing I do not like here is that there is no guarantee there is no typo between the types and the constants. Maybe each constant could also be explicitly given its type?
|
@justinfagnani very good point - this support outputs a value that represents the URL of the output bundle in whatever environment it is run. In terms of asset loads / relocations I think perhaps using an asset API plugin is the better approach for that - |
|
@guybedford we have a transform that we run before files get to rollup that turns {...import.meta, url: new URL(relativePathFromBundleToOriginal, import.meta.url).href}Which preserves the original URL. For much of our code at least, it's very important that |
92fcdbe to
0457801
Compare
|
@justinfagnani interesting, yeah this also catches on some of the differences between workflows one might expect. Library authors may just want to output an Perhaps there are some customization options that would work here. We could even open this up as a plugin hook for deeper flexibility like we do with dynamic import. I think we should definitely look into this for a future PR. |
|
Will put some further thought to this. I think it's definitely worth noting though that use cases around import.meta.url are not currently fully fleshed out, and that we are still working on the approaches - it is under an experimental flag, but this makes me think perhaps we should give it its own experimental flag - |
I think library authors want the location preserving behavior. The use case is something like: const css = await (await fetch(new URL('./my-component.css', import.meta.url).href)).text();where you'll want the URL to point back to the original location so that In Polymer we use this to fix up URLs in component templates for similar purposes. |
|
@justinfagnani I completely appreciate your use case. Imagine too building a single-file library where you want that exact output, but your source file is in It does seem like we need some options around the normalization behaviour to cater to different scenarios. |
lukastaegert
left a 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.
From my side this looks good now! As for the comments by @justinfagnani:
Those are definitely valid use cases. I would still prefer the currently implemented approach as the default for two reasons:
- The current approach will not transform
import.meta.urlfor esm builds. For all other formats, it will be polyfilled in a way that aims to mimic the behaviour in the esm case. - In order to get information about the build before bundling there are different, though not necessarily elegant, ways of achieving this. Namely as all directory information is available before bundling, it would be possible to inject the meta information via a custom transformer plugin that replaces e.g.
__dirnamevia the actual path or similar things. Information on the runtime environment, on the other hand, cannot be provided in such a way.
Still it might be interesting to see what other use-cases people have here and maybe provide a useful plugin hook to support those.
0457801 to
78c6def
Compare
|
Was some way added to allow hooking into the Basically I'd like to do pretty much the same thing as @justinfagnani suggested in his comments. That being having |
|
@Jamesernator no this is still pending some work. Perhaps a |
|
When would What would other options even be? |
|
The other option is to wait until a clear solution presents itself, instead of rushing to create a feature that may incur technical debt in future :) |
|
How about a hook like the following: resolveImportMetaUrl (id: string, outFile: string) {
return resolved;
}where if the return value is an absolute URL, it is used directly, and if it is a relative URL it is assumed relative to the outFile. In this scenario, the following hook would effectively be the default as we have today: resolveImportMetaUrl (id: string, outFile: string) {
return './' + path.basename(outFile);
}@justinfagnani @Jamesernator if that sounds like it might solve your use case, a PR along these lines is welcome, or I will see if I can get to this as well. |
This property is already supported in both browsers and NodeJS under --experimental-modules.
This implements
import.meta.urlunder theexporimentalDynamicImportoption (both on theimportreserved identifier!).The rewriting is designed to be as flexible as possible for all output formats, and through using the global URL, we can remain pretty spec compliant as well.
In the process I refactored NodeTypes to be inlined as strings in the build, which reduces the build size of Rollup. The issue with the types here is that this requires distinguishing between uses of NodeType for a type and for an actual string. So I duplicated these as NodeType.tMemberExpression for the TypeScript type and NodeType.MemberExpression for the JS string. This then allows tree-shaking the unused literals in the output build, reducing build size by a couple of KB.
I also inlined the dynamic import mechanism code a little too, removing some of the logic from Chunk.ts.
Example: