Non-enumerable and inherited properties are inconsistently imported

[evanw]

• Rollup Plugin Name: @rollup/plugin-commonjs
• Rollup Plugin Version: 16.0.0
• Rollup Version: 2.33.2
• Operating System (or Browser): macOS
• Node Version: v14.15.0
• Link to reproduction (⚠️ read below): https://repl.it/@EvanWallace1/rollup-commonjs-import-inconsistency

// input.js
import * as test from './test'

console.log('nonEnumerable (direct):', test.nonEnumerable)
console.log('nonEnumerable (indirect):', test[Math.random() < 2 && 'nonEnumerable'])

console.log('inherited (direct):', test.inherited)
console.log('inherited (indirect):', test[Math.random() < 2 && 'inherited'])

// test.js
module.exports = Object.create({ inherited: true })
Object.defineProperty(module.exports, 'nonEnumerable', {value: true})

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 like test.property.

Expected Behavior

An import should either be always imported or never imported, regardless of the import syntax used.

Either this:

nonEnumerable (direct): undefined
nonEnumerable (indirect): undefined
inherited (direct): undefined
inherited (indirect): undefined

Or this:

nonEnumerable (direct): true
nonEnumerable (indirect): true
inherited (direct): true
inherited (indirect): true

Actual Behavior

Whether a property can be imported or not depends on the syntax used:

nonEnumerable (direct): true
nonEnumerable (indirect): undefined
inherited (direct): true
inherited (indirect): undefined

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

[guybedford]

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.

[guybedford]

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

[evanw]

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 module.exports = class { static property() {} }, so it's not that unusual. In that case property is non-enumerable.

[guybedford]

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.

[lukastaegert]

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 module.exports (called __moduleExports) and designating it as the synthetic fallback namespace. This has two effects:

• You can import anything from a commonjs module as a named export, even if the named export detection logic did not detect it.
• We support live-bindings for all exports. If the export is detected, then all accesses to the exports property are rewritten to support this. If it is not detected, Rollup rewrites imports as property accesses for the synthetic namespace. This slightly diverges from how Node does it as far as I know as here, named exports are only snapshots (correct me if I am wrong @guybedford).

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

import {bar} from 'foo';
console.log(bar);

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 Object.assign(Object.create(null), syntheticNamespace, actualNamespace). Of course, this would skip inherited and non-enumerable properties.

I have pondered (but did not find time yet), to improve this logic by using a custom helper that instead uses Object.getOwnPropertyDescriptors so that e.g. getters are not destroyed in the process. That would then also include non-enumerable properties. But I do not see Rollup ever supporting inherited properties in dynamic namespaces, and this also feels very sub-optimal performance-wise: You would always copy everything from Object.prototype for instance, unless you give the namespace a prototype, which would cause the exports to know longer show up for Object.keys etc. Unfortunately, there is no way to get rid of those for non-dynamic property accesses.

[stale]

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