CommonJS import from a Module package too inflexible

[k2snowman69]

I originally opened this over in nodejs/node#39052 and was asked to move this here. Using the same structure since it has quite a bit of detail already.

• Version: v14.17.1
• Platform: Microsoft Windows NT 10.0.19042.0 x64
• Subsystem:

What steps will reproduce the bug?

Whenever a consuming npm package is type: module it seems to be much picker about importing packages of which distribute cjs style code via import { bar } from "../lib/factory.js";. This limitation prevents consuming libraries from using type: module until all the dependent libraries are fixed and corrected which slows adoption.

To give an example I've created a repository https://github.com/k2snowman69/bug-nodejs-import-cjs which has two folders

• lib - Contains trimmed down versions of three different styles of cjs outputs I've found (glob, dropbox and tslib). I stopped at three because it got the point across
• use-lib - an ESM package that consume the three different styles of CJS outputs

How often does it reproduce? Is there a required condition?

Every time across multiple versions of node

What is the expected behavior?

When running any of the following:

• node .\use-lib\use-define-property.js
• node .\use-lib\use-factory.js
• node .\use-lib\use-function.js

All three succeed in executing. This would mean that even though a dependency of my project might not be written in ESM or be rewritten using the Object.defineProperty style of CJS, that I'm able to use ESM style for my particular project though lose on some of the major benefits if that dependency were written in ESM.

What do you see instead?

If the commonjs library dist output uses the Object.defineProperty to build it's output, it works successfully, however the other two solutions seem to break.

PS D:\Projects\bug-nodejs-import-cjs> node .\use-lib\use-define-property.js
bar

PS D:\Projects\bug-nodejs-import-cjs> node .\use-lib\use-factory.js
file:///D:/Projects/bug-nodejs-import-cjs/use-lib/use-factory.js:1
import { bar } from "../lib/factory.js";
         ^^^
SyntaxError: Named export 'bar' not found. The requested module '../lib/factory.js' is a CommonJS module, which may not support all module.exports as named exports.
CommonJS modules can always be imported via the default export, for example using:

import pkg from '../lib/factory.js';
const { bar } = pkg;

←[90m    at ModuleJob._instantiate (internal/modules/esm/module_job.js:120:21)←[39m
←[90m    at async ModuleJob.run (internal/modules/esm/module_job.js:165:5)←[39m
←[90m    at async Loader.import (internal/modules/esm/loader.js:177:24)←[39m
←[90m    at async Object.loadESM (internal/process/esm_loader.js:68:5)←[39m

PS D:\Projects\bug-nodejs-import-cjs> node .\use-lib\use-function.js
file:///D:/Projects/bug-nodejs-import-cjs/use-lib/use-function.js:1
import { bar } from "../lib/function.js";
         ^^^
SyntaxError: Named export 'bar' not found. The requested module '../lib/function.js' is a CommonJS module, which may not support all module.exports as named exports.
CommonJS modules can always be imported via the default export, for example using:

import pkg from '../lib/function.js';
const { bar } = pkg;

←[90m    at ModuleJob._instantiate (internal/modules/esm/module_job.js:120:21)←[39m
←[90m    at async ModuleJob.run (internal/modules/esm/module_job.js:165:5)←[39m
←[90m    at async Loader.import (internal/modules/esm/loader.js:177:24)←[39m
←[90m    at async Object.loadESM (internal/process/esm_loader.js:68:5)←[39m

Additional information

This is my first bug in such a public space and I'm a bit nervous, feel free to ask me to reorganize or cleanup the original description to make it easier to understand. Any suggestions would be great, I'm trying to get better at writing issues!

[guybedford]

Thanks for reporting the issue, and as you can likely tell there is quite a bit of complex reasoning for how we got here.

The short answer is that the cases you're talking about supporting like function obj () {}; obj.foo = ... are all cases where it's impossible to tell foo is an export before the CommonJS has executed.

The restriction of needing to work out what a CommonJS module will export before we can execute it is one that was imposed by a combination of requirements from the ECMAScript modules spec itself to Node.js requirements, that were all fought and argued extensively.

While it is possible in theory to support these cases via static analysis, the other constraint this project has is to extract the list of exports incredibly quickly so there is no execution slowdown. More advanced code analysis to determine obj is a function that is later exported, that has properties applied, is simply out of the scope of this project unfortunately.

There are likely simple cases we can improve - eg we don't currently support module.exports = { a: 1 }, simply because a number on the value side of an object expression isn't yet supported by the parser. So if you hit simpler cases where your eyes don't need to understand JavaScript execution to easily see what is exported (about the level of this project), then we can certainly add support for those.

Let me know if that answers your question and sorry we can't do better than this.

[k2snowman69]

I think it answers my questions and makes sense why an eval is completely out of the question. Further I wondered if we could use the *.d.ts if available to try aid in the populating of exports. I think this could be more efficient than running eval on the js file itself, however still goes against your goals and probably would have perf impact as it's still not just a regex expression. Cutting that as an option as a whole then.

Below you'll find some notes I had per package and why they didn't work based on your answers. This is mostly for anyone who comes to see this issue later. On this, I definitely think there is a bug with the enzyme scenario and I even added the unit test code in that example. Hopefully it helps you quickly be able to come up with a fix (I think it's related to the module.exports = { a: 1 } case).

At this point, feel free to close this issue or keep it open to fix the enzyme example I provided. However I think you've answered all my questions so thank you!

chalk

unpkg link to package contents

Reason this library is unable to parse the commonjs contents is because the exports are defined as such module.exports = chalk; which to your point, requires an eval to determine the values.

However, this package does provide a index.d.ts... which is very complicated and would not be useful. That idea's out the door too.

commander

unpkg link to package contents

Reason this library is unable to parse is because the exports line is as such: exports = module.exports = new Command();. Effectively same issue as chalk.

glob

unpkg link to package contents

Same as chalk and commander

enzyme

unpkg link to package contents

This one actually should be parseable. The exports are as follows:

module.exports = {
  render: _render2["default"],
  shallow: _shallow2["default"],
  mount: _mount2["default"],
  ShallowWrapper: _ShallowWrapper2["default"],
  ReactWrapper: _ReactWrapper2["default"],
  configure: _configuration.merge,
  EnzymeAdapter: _EnzymeAdapter2["default"],
};

I created a unit test for this particular file and found that it's only picking up the first key render however the rest of the keys seem to be missing.

  test("complex values", () => {
    const { exports, reexports } = parse(`
    var _ReactWrapper = require('./ReactWrapper');
    var _ReactWrapper2 = _interopRequireDefault(_ReactWrapper);
    var _ShallowWrapper = require('./ShallowWrapper');
    var _ShallowWrapper2 = _interopRequireDefault(_ShallowWrapper);
    var _EnzymeAdapter = require('./EnzymeAdapter');
    var _EnzymeAdapter2 = _interopRequireDefault(_EnzymeAdapter);
    var _mount = require('./mount');
    var _mount2 = _interopRequireDefault(_mount);
    var _shallow = require('./shallow');
    var _shallow2 = _interopRequireDefault(_shallow);
    var _render = require('./render');
    var _render2 = _interopRequireDefault(_render);
    var _configuration = require('./configuration');
    
    function _interopRequireDefault(obj) { return obj && obj.__esModule ? obj : { 'default': obj }; }
    
    module.exports = {
      render: _render2['default'],
      shallow: _shallow2['default'],
      mount: _mount2['default'],
      ShallowWrapper: _ShallowWrapper2['default'],
      ReactWrapper: _ReactWrapper2['default'],
      configure: _configuration.merge,
      EnzymeAdapter: _EnzymeAdapter2['default']
    };
    `);
    assert.equal(exports[0], "render");
    assert.equal(exports[1], "shallow");
    assert.equal(exports[2], "mount");
    assert.equal(exports[3], "ShallowWrapper");
    assert.equal(exports[4], "ReactWrapper");
    assert.equal(exports[5], "configure");
    assert.equal(exports[6], "EnzymeAdapter");
    assert.equal(exports.length, 7);
  });

tslib

unpkg link to package contents

Seems to work as expected which is great!

[guybedford]

Agreed it would be nice to parse the enzyme one. There's actually a way we could support this in the parser I think, although it would be an interesting new parser approach... are you interested in working on the parser?

[k2snowman69]

As much as I'd love to I simply wont have time. This is only one of multiple issues I'm running into while trying to move my org to be able to use ESM. Currently hunting and debugging each one to open tickets in all the respective projects (as you can see from the investigation above I'm going package by package) is very time consuming.

I'll try to loop back when time opens up in case it hasn't been tackled but I'd estimate months out at this point.

[guybedford]

Ok I've posted #58 which we can leave open to track the specific support for whenever whomever has time! Thanks again for the comments.