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

Scope of imported variables (nested imports) #8021

Closed
klaussner opened this issue Nov 5, 2016 · 3 comments
Closed

Scope of imported variables (nested imports) #8021

klaussner opened this issue Nov 5, 2016 · 3 comments
Assignees
Milestone

Comments

@klaussner
Copy link
Member

@klaussner klaussner commented Nov 5, 2016

I was trying to use nested imports and noticed that the scoping doesn't work as described in reify's README.md. In particular, a variable imported inside an if block has the scope of a var declaration, not a let declaration.

For example:

if (true) {
  import value from './file.js';
}

console.log(value);

logs the value of the imported file (instead of undefined) because the code is transpiled to:

if (true) {
  var value;

  module.import('./file.js', {
    "default": function(v) {
      value = v
    }
  });
}

console.log(value);

This leads to problems when using the same name for imports more than once, in different block scopes:

if (true) {
  import value from '/imports/a.js';
  console.log(value);
} else {
  import value from '/imports/b.js';
  console.log(value);
}
=> Errors prevented startup:

   While processing files with ecmascript (for target web.browser):
   client/client.js:5:9: /client/client.js: Duplicate declaration "value"
@klaussner klaussner added this to the Release 1.4.2.1 milestone Nov 7, 2016
@benjamn
Copy link
Member

@benjamn benjamn commented Nov 7, 2016

The real reason for this is that Reify sits at the very end of the compiler toolchain, just before Node runs its version of eval if you're using the require("reify") strategy, or after all other Babel transforms if you're using it as part of meteor-babel.

Since Reify doesn't have the ability to transpile let declarations for older versions of Node, it seems like a bad idea for it to be generating new let declarations, if there's any chance that the JS engine does not support let natively. Unfortunately, Node 4 still doesn't support let outside of strict mode, so it's not enough to know that you're using Node 4. You might think that using import implies you're in a module, which implies strict mode, but in Meteor we let people opt into "use strict" for their modules, in case they're still using auto-imported "global" variables.

With all of that said, I think we may be able to fix this in https://github.com/meteor/babel by moving the Reify compilation step earlier in the Babel plugin pipeline, even if we can't fix it once and for all in Reify itself. Some Babel transforms insert additional import declarations, so we would have to reenable the Babel modules transform, but those import declarations won't need (or benefit from) Reify because they'll all be at the top level, and Babel will turn them into require calls. If this works, then Babel can transpile the let declarations created by Reify, and Reify will only have to handle the import declarations from your source code (not the ones Babel itself inserts).

benjamn added a commit to benjamn/reify that referenced this issue Nov 7, 2016
Part of a solution to this issue reported by @klaussner:
meteor/meteor#8021
@benjamn
Copy link
Member

@benjamn benjamn commented Nov 7, 2016

A fix for this issue will probably also fix #7662.

benjamn added a commit to meteor/babel that referenced this issue Nov 8, 2016
@benjamn
Copy link
Member

@benjamn benjamn commented Nov 8, 2016

Though tests were passing on #7975, a complete solution for this issue is going to have to wait for 1.4.3, since moving Reify before Babel means Reify must now handle syntax like

export const { foo, bar } = baz();

In other words, Reify really needs to sit somewhere in between transforms like the one that handles destructuring variable declarations and the one that handles let and const declarations. And given that those two transforms are closely related, the precise position of Reify in the transform pipeline is probably more important than I realized, which is mostly to say: Reify needs to become a proper Babel plugin.

The good news is that this issue (and #7662) will definitely be fixed when that happens.

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

Successfully merging a pull request may close this issue.

None yet
2 participants