-
-
Notifications
You must be signed in to change notification settings - Fork 592
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
Non-enumerable and inherited properties are inconsistently imported #649
Comments
Thanks for researching this. I don't actually think this inconsistency is a conern. In Node.js the only requirement is that the property is an own property, and it does do this filtering. Enumerability isn't a property that is filtered. So I don't think any changes need to be made here in the emission. |
(For example __esModule is non-enumerable but is always emitted in Node.js as an export without being a special case, and that reemission is in fact important to some interop scenarios) |
Yes it's fine to close this issue if it works the way you want it to. This issue is mainly just an FYI. Personally I find it really weird that exports would sometimes be there and sometimes not be there depending on the import syntax used. For example, it means that if you refactored a copied-and-pasted sequence of named properties into a dynamic property access, it could break in weird non-intuitive ways: // From this:
load(ns.prop1)
load(ns.prop2)
load(ns.prop3)
...
// To this:
for (let prop of ['prop1', 'prop2', 'prop3', ...])
load(ns[prop]) I hit this problem myself when testing what Rollup supported. My first approach was to do a for loop and that initially led me to think Rollup didn't support any of these cases. The real answer is that it actually does support them, but only if you use a certain import syntax. FWIW this situation comes up when a CommonJS module does |
Let us give @lukastaegert time to respond on this issue first, I very much from from the Node.js angle here at this point which defaults my answer somewhat (excuse the pun :P), but I'm sure he will have an interesting perspective to add as well. As far as I'm aware RollupJS actually does a static analysis similar to what Node.js does at this point by examining the AST to determine the exports instead of doing it at runtime, but @lukastaegert has been changing the logic quite a bit recently so I'm not sure what the current pattern is exactly. See also https://github.com/guybedford/cjs-module-lexer used in Node.js. |
The problem here is that Rollup works by converting everything to ESM first. On the other hand, Rollup also supports a pattern called a "synthetic namespace" which means that a plugin can designate a variable as a fallback namespace in case something is imported that is not part of the actual ESM namespace. The commonjs plugin uses this by always exporting
This brings a problem though in situations where we need to reify a namespace object. In your examples, this was only necessary if you used a non-trivial computed property access as in all other situations, Rollup core would treat e.g. import * as foo from 'foo';
console.log(foo.bar); as
internally. However in the case you import something dynamic, Rollup needs to create a namespace object. Usually, this is not a problem, but in case a synthetic namespace is involved, you need to mix the synthetic namespace into the actual namespace so that you still find the fallbacks. At the moment, this is done very crudely via an I have pondered (but did not find time yet), to improve this logic by using a custom helper that instead uses |
Hey folks. This issue hasn't received any traction for 60 days, so we're going to close this for housekeeping. If this is still an ongoing issue, please do consider contributing a Pull Request to resolve it. Further discussion is always welcome even with the issue closed. If anything actionable is posted in the comments, we'll consider reopening it. ⓘ |
Note that the code above uses
test[Math.random() < 2 && 'property']
to represent a dynamic property access. It's not the intention of this issue to special-case that syntax to behave liketest.property
.Expected Behavior
An import should either be always imported or never imported, regardless of the import syntax used.
Either this:
Or this:
Actual Behavior
Whether a property can be imported or not depends on the syntax used:
Additional Information
I'm working on improving esbuild's compatibility with other bundlers. This inconsistency tripped me up in the past when trying to figure out what forms of importing Rollup supports. Ideally it would be consistent. I'm guessing no one else has pointed this out yet so I figured I should log an issue. Here's the context: evanw/esbuild#532 (comment).
The text was updated successfully, but these errors were encountered: