Skip to content

Conversation

@guybedford
Copy link
Contributor

@guybedford guybedford commented Apr 27, 2018

This property is already supported in both browsers and NodeJS under --experimental-modules.

This implements import.meta.url under the exporimentalDynamicImport option (both on the import reserved 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:

console.log(import.meta.url);
// -> es:
console.log(import.meta.url);
// -> cjs:
console.log(new (typeof URL !== 'undefined' ? URL : require('url').URL)('file:' + __filename).href)
// -> system:
console.log(module.meta.url);
// -> global:
console.log((typeof document !== 'undefined' ? document.currentScript && document.currentScript.src || location.href : new URL('file:' + __filename).href));

@justinfagnani
Copy link

@guybedford Does this transform preserve the URL of the original module, rather than return the URL of the bundle?

Copy link
Member

@lukastaegert lukastaegert left a 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

return node.type === NodeType.MetaProperty;
}

export default interface MetaProperty extends Node {
Copy link
Member

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).

Copy link
Contributor Author

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?

Copy link
Member

@lukastaegert lukastaegert May 14, 2018

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.

storeName: true,
contentOnly: true
});
} else if (isMetaProperty(this.object) && this.object.meta.name === 'import') {
Copy link
Member

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

Copy link
Member

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
};

Copy link
Member

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 👍

cjs: `new (typeof URL !== 'undefined' ? URL : require('url').URL)('file:' + __filename).href`,
iife: globalImportMetaUrlMechanism,
umd: globalImportMetaUrlMechanism
};
Copy link
Member

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?

export const VariableDeclarator = 'VariableDeclarator';
export const VariableDeclaration = 'VariableDeclaration';
export const WhileStatement = 'WhileStatement';
export const YieldExpression = 'YieldExpression';
Copy link
Member

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?

@guybedford
Copy link
Contributor Author

@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 - import assetPath from './asset.ext'. Would be interested to hear your thoughts.

@justinfagnani
Copy link

@guybedford we have a transform that we run before files get to rollup that turns import.meta into:

{...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 import.meta give the same answer with and without bundling. Browsers don't support import assetPath from './asset.ext', so we can't do that.

@guybedford
Copy link
Contributor Author

@justinfagnani interesting, yeah this also catches on some of the differences between workflows one might expect. Library authors may just want to output an import.meta.url statement, treating the build folder as relocatable, while application authors may want something closer to what you describe.

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.

@guybedford
Copy link
Contributor Author

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 - experimentaImportMeta, so that we can stabilize it separately to the code splitting work.

@justinfagnani
Copy link

Library authors may just want to output an import.meta.url statement, treating the build folder as relocatable, while application authors may want something closer to what you describe.

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 ./my-component.css resolves correctly.

In Polymer we use this to fix up URLs in component templates for similar purposes.

@guybedford
Copy link
Contributor Author

@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 src/app.js being built into dist/build.js, where you know it will be fetching from dist/build.css.

It does seem like we need some options around the normalization behaviour to cater to different scenarios.

Copy link
Member

@lukastaegert lukastaegert left a 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.url for 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. __dirname via 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.

@lukastaegert lukastaegert added this to the 0.59.0 milestone May 14, 2018
@lukastaegert lukastaegert merged commit 78c6def into master May 14, 2018
@lukastaegert lukastaegert deleted the import-meta-url branch May 14, 2018 05:26
@Jamesernator
Copy link

Was some way added to allow hooking into the import.meta transform? As I'd like to use Rollup purely as an optimization that doesn't change any (major) semantics.

Basically I'd like to do pretty much the same thing as @justinfagnani suggested in his comments. That being having import.meta.url always be the same as where the file would've been if the module were loaded without bundling.

@guybedford
Copy link
Contributor Author

@Jamesernator no this is still pending some work. Perhaps a importMetaUrl: 'relative' option could be implemented. PRs welcome.

@justinfagnani
Copy link

When would importMetaUrl: 'relative' ever not be the right option? We want module to work when they're not bundled, so import.meta.url has to point to where it would have before bundling, no?

What would other options even be?

@guybedford
Copy link
Contributor Author

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 :)

@guybedford
Copy link
Contributor Author

guybedford commented Jul 31, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants