Skip to content
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

Allow import.meta.* instead of only import.meta.url #3151

Closed
dmail opened this issue Oct 11, 2019 · 9 comments
Closed

Allow import.meta.* instead of only import.meta.url #3151

dmail opened this issue Oct 11, 2019 · 9 comments

Comments

@dmail
Copy link
Contributor

dmail commented Oct 11, 2019

Expected Behavior / Situation

I would like that the following code

import.meta.resolve('./file.js')

To output something like this for systemjs format.

System.register([], function (exports, module) {
  return {
    execute: function () {
      module.meta.resolve("./file.js");
    }
  }
});

Actual Behavior / Situation

The output has no module, resulting in a ReferenceError: module is not defined.

System.register([], function (exports) {
  return {
    execute: function () {
      module.meta.resolve("./file.js");
    }
  }
});

Modification Proposal

Make this if less strict.

if (metaProperty === 'url') {

Adding || metaProperty === 'resolve' works for me.


Note by maintainers (by @lukastaegert)

🙋‍♀️ Want to get involved and address this? This issue is likely easy to address and a good starting point to get familiar with the Rollup code base.

The proposed solution is to reverse the linked if statement so that all non-Rollup-internal cases are handled like the generic URL case. It should probably be checked if this assumption makes sense for all formats.

As a test, adding one to the form directory would make sense, as these render the code in all available formats. Look at the other tests how those are written. To validate the code is actually runnable, it could make sense to also add a test to the function directory as those tests compile to CommonJS and actually run the code, but this is optional.

@lukastaegert
Copy link
Member

I see your point and it definitely makes sense to have a fallback for other properties. I think a prudent change could be to reverse the if statement: If none of the special cases internal to Rollup are encountered, then ALWAYS add the additional accessed globals.

@lukastaegert
Copy link
Member

PR welcome, I added additional notes to the issue description.

@dmail
Copy link
Contributor Author

dmail commented Oct 11, 2019

I have created the PR to initiate the work.

It would match my need but I'm not very confortable with

'use strict';

undefined('./foo.js');

being the expected output for commonjs format 😅

@lukastaegert
Copy link
Member

See my comment on your PR, I think it is a reasonable compromise.

@lukastaegert
Copy link
Member

People can still change this by implementing resolveImportMeta

@lukastaegert
Copy link
Member

resolved via #3152

@dmail
Copy link
Contributor Author

dmail commented Oct 17, 2019

Thank you very much 😍

@lukastaegert
Copy link
Member

Actually a bug. Fix here: #3282

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

No branches or pull requests

3 participants