Handle falsy synthetic namespaces

[lukastaegert]

This PR contains:

• bugfix
• feature
• refactor
• documentation
• other

Are tests included?

• yes (bugfixes and features will not be merged without tests)
• no

Breaking Changes?

• yes (breaking changes will not be merged unless absolutely necessary)
• no

List any relevant issue numbers:
resolves #4245

Description

The commonjs plugin always uses module.exports as the "fallback namespace" for modules. In the rare case where this is either null or undefined, this will cause Rollup's helper to error as that one assumes the namespace to be an object. This adds a small check to the helper to account for this case.

[github-actions]

Thank you for your contribution! ❤️

You can try out this pull request locally by installing Rollup via

npm install rollup/rollup#handle-falsy-synthetic-namespace

or load it into the REPL:
https://rollupjs.org/repl/?pr=4247

[codecov]

Codecov Report

Merging #4247 (9711ac7) into master (cc4fc58) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4247      +/-   ##
==========================================
- Coverage   98.39%   98.37%   -0.02%     
==========================================
  Files         203      203              
  Lines        7342     7272      -70     
  Branches     2088     2081       -7     
==========================================
- Hits         7224     7154      -70     
  Misses         58       58              
  Partials       60       60              

Impacted Files	Coverage Δ
src/utils/interopHelpers.ts	100.00% <ø> (ø)
src/ast/nodes/Identifier.ts	100.00% <100.00%> (ø)
src/ast/nodes/PropertyDefinition.ts	88.88% <0.00%> (-2.03%)	⬇️
src/utils/timers.ts	56.71% <0.00%> (-0.64%)	⬇️
cli/run/watch-cli.ts	91.89% <0.00%> (-0.22%)	⬇️
src/ast/nodes/TryStatement.ts	95.45% <0.00%> (-0.20%)	⬇️
cli/run/loadConfigFile.ts	96.07% <0.00%> (-0.15%)	⬇️
src/utils/generateCodeSnippets.ts	97.29% <0.00%> (-0.08%)	⬇️
src/watch/watch.ts	99.05% <0.00%> (-0.01%)	⬇️
cli/run/batchWarnings.ts	99.19% <0.00%> (-0.01%)	⬇️
... and 40 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cc4fc58...9711ac7. Read the comment docs.

[kzc]

Noticed spaces around e && in the compact tests results...

function _mergeNamespaces(n, m){m.forEach(function(e){e && Object.keys(e).forEach(function(k){if(k!=='default'&&!(k in n)){var d=Object.getOwnPropertyDescriptor(e,k);Object.defineProperty(n,k,d.get?d:{enumerable:true,get:function(){return e[k]}});}})});return Object.freeze(n);}

Is it worth bothering with ${_}?

--- a/src/utils/interopHelpers.ts
+++ b/src/utils/interopHelpers.ts
@@ -255,5 +255,5 @@ const loopOverNamespaces = (
        return (
                `m.forEach(${left}` +
-               `Object.keys(e).forEach(${getFunctionIntro(['k'], {
+               `e${_}&&${_}Object.keys(e).forEach(${getFunctionIntro(['k'], {
                        isAsync: false,
                        name: null

[kzc]

It's hard to discern from the large patch, but is there a test for the falsy module.exports case for the generatedCode es2015 scenario? I appreciate it worked correctly in #4245, but it would be useful to exercise it.

[guybedford]

I'd be interested to know how it's possible in core for a namespace to be null though, that seems to break the ES module spec!?

[kzc]

For a string, Object.keys('asdf') will loop over the individual indices, is that case previously guarded?

Good point. I recall seeing individual characters of a string exported now that you mention it. The es2015 version of _mergeNamespaces may have the same issue as well.

I'd be interested to know how it's possible in core for a namespace to be null though, that seems to break the ES module spec!?

It's not an ES module thing, per se, it is the result of commonjs converted to an ES module for themodule.exports = undefined case. The original bug report had a jquery commonjs extension that altered the jquery commonjs module but had nothing interesting to return for itself. So such a commonjs module would have no contribution to a merged namespace.

Edit: Specifically this is related to the rollup syntheticNamedExports feature to automatically export commonjs properties to ES modules.

[kzc]

For a string, Object.keys('asdf') will loop over the individual indices

Example:

$ echo 'module.exports = "Abc";' > abc.js
$ echo 'import("./abc.js").then(m=>console.log(JSON.stringify(m,null,2)));' > dyn-import-abc.js

Expected:

$ node dyn-import-abc.js 
{
  "default": "Abc"
}

$ esbuild dyn-import-abc.js --bundle --platform=node | node
{
  "default": "Abc"
}

es5 helpers:

$ rollup dyn-import-abc.js --generatedCode es5 -p node-resolve -p commonjs -f cjs --exports auto -d dist

dyn-import-abc.js → dist...
created dist in 69ms

$ node dist/dyn-import-abc.js 
{
  "0": "A",
  "1": "b",
  "2": "c",
  "default": "Abc"
}

es2015 helpers:

$ rollup dyn-import-abc.js --generatedCode es2015 -p node-resolve -p commonjs -f cjs --exports auto -d dist

dyn-import-abc.js → dist...
created dist in 68ms

$ node dist/dyn-import-abc.js 
{
  "0": "A",
  "1": "b",
  "2": "c",
  "default": "Abc"
}

[kzc]

With @guybedford's suggestion above:

--- a/src/utils/interopHelpers.ts
+++ b/src/utils/interopHelpers.ts
@@ -244,7 +244,8 @@ const loopOverNamespaces = (
                return (
                        `for${_}(var i${_}=${_}0;${_}i${_}<${_}m.length;${_}i++)${_}{${n}` +
                        `${t}${t}${cnst} e${_}=${_}m[i];${n}` +
-                       `${t}${t}for${_}(${cnst} k in e)${_}${body}${n}${t}}`
+                       `${t}${t}if (e && (typeof e === 'object' || typeof e === 'function')) {` +
+                       `${t}${t}for${_}(${cnst} k in e)${_}${body}${n}${t}${t}}${n}${t}}`
                );
        }
        const [left, right] = getDirectReturnFunction(['e'], {
@@ -254,7 +255,7 @@ const loopOverNamespaces = (
        });
        return (
                `m.forEach(${left}` +
-               `Object.keys(e).forEach(${getFunctionIntro(['k'], {
+               `e && (typeof e === 'object' || typeof e === 'function') && Object.keys(e).forEach(${getFunctionIntro(['k'], {
                        isAsync: false,
                        name: null
                })}${body})${right});`

es5 helpers:

$ rollup dyn-import-abc.js --generatedCode es5 -p node-resolve -p commonjs -f cjs --exports auto -d dist --silent && node dist/dyn-import-abc.js
{
  "default": "Abc"
}

es2015 helpers:

$ rollup dyn-import-abc.js --generatedCode es2015 -p node-resolve -p commonjs -f cjs --exports auto -d dist --silent && node dist/dyn-import-abc.js
{
  "default": "Abc"
}

Edit 1: Added parentheses around the es5 helper condition.
Edit 2: Rearranged the parens to: e && (typeof e === 'object' || typeof e === 'function')
Note: The tab/newline formatting of the es2015 helper change is bad, but the code works.

[lukastaegert]

I'd be interested to know how it's possible in core for a namespace to be null though, that seems to break the ES module spec!?

Synthetic namespaces are not really namespaces but it is something where a plugin says: If a named import is not found, take a property of that exported object. Unfortunately, there is no runtime check that the export is really an object. For "real" namespaces, this cannot happen.
The other scenario where the helper is used is merging internal and external namespaces. With different interop settings, it might be possible that a namespace is not an object either.

That being said, I am not really sure we need to fix the string case. It bloats helpers for everyone, it is not necessary to fix a crash, and as what we are doing does not follow any spec here anyway, this is kind of a gray area. The namespace object will have additional properties, but all expected properties will be present (which in the commonjs case will just be "default"). But I can add the additional code if you deem it critical enough.

[kzc]

I'd vote in favor of correctness and matching NodeJS's and esbuild's functionality despite the minimal extra code, which is a fixed size.

The failing test case above is a not unusual scenario where a CJS module returns something (perhaps as result of a calculation) that another module processes as JSON verbatim.

esbuild output shown for comparison:

$ esbuild dyn-import-abc.js --bundle --platform=node
var __create = Object.create;
var __defProp = Object.defineProperty;
var __getOwnPropDesc = Object.getOwnPropertyDescriptor;
var __getOwnPropNames = Object.getOwnPropertyNames;
var __getProtoOf = Object.getPrototypeOf;
var __hasOwnProp = Object.prototype.hasOwnProperty;
var __markAsModule = (target) => __defProp(target, "__esModule", { value: true });
var __commonJS = (cb, mod) => function __require() {
  return mod || (0, cb[Object.keys(cb)[0]])((mod = { exports: {} }).exports, mod), mod.exports;
};
var __reExport = (target, module2, desc) => {
  if (module2 && typeof module2 === "object" || typeof module2 === "function") {
    for (let key of __getOwnPropNames(module2))
      if (!__hasOwnProp.call(target, key) && key !== "default")
        __defProp(target, key, { get: () => module2[key], enumerable: !(desc = __getOwnPropDesc(module2, key)) || desc.enumerable });
  }
  return target;
};
var __toModule = (module2) => {
  return __reExport(__markAsModule(__defProp(module2 != null ? __create(__getProtoOf(module2)) : {}, "default", module2 && module2.__esModule && "default" in module2 ? { get: () => module2.default, enumerable: true } : { value: module2, enumerable: true })), module2);
};

// abc.js
var require_abc = __commonJS({
  "abc.js"(exports2, module2) {
    module2.exports = "Abc";
  }
});

// dyn-import-abc.js
Promise.resolve().then(() => __toModule(require_abc())).then((m) => console.log(JSON.stringify(m, null, 2)));

to the proposed rollup fix:

$ rollup dyn-import-abc.js --generatedCode es5 -p node-resolve -p commonjs -f cjs --exports auto

dyn-import-abc.js → stdout...

//→ dyn-import-abc.js:
'use strict';

Promise.resolve().then(function () { return require('./abc-09f6dfe3.js'); }).then(function (n) { return n.abc; }).then(m=>console.log(JSON.stringify(m,null,2)));

//→ abc-09f6dfe3.js:
'use strict';

function _mergeNamespaces(n, m) {
	m.forEach(function (e) {
		e && (typeof e === 'object' || typeof e === 'function') && Object.keys(e).forEach(function (k) {
			if (k !== 'default' && !(k in n)) {
				var d = Object.getOwnPropertyDescriptor(e, k);
				Object.defineProperty(n, k, d.get ? d : {
					enumerable: true,
					get: function () { return e[k]; }
				});
			}
		});
	});
	return Object.freeze(n);
}

var abc = "Abc";

var abc$1 = /*#__PURE__*/Object.freeze(/*#__PURE__*/_mergeNamespaces({
	__proto__: null,
	default: abc
}, [abc]));

exports.abc = abc$1;

[lukastaegert]

I added special handling for strings but went for typeof e !== string, which is more compact and also makes clear why we are doing this in the first place. All other types appear to work as expected.

[kzc]

We failed to consider the default export of an array, whose typeof is "object".
Should probably add && !Array.isArray(e) as well for good measure...

$ cat dyn-import-arr.js 
import("./arr.js").then(m=>console.log(JSON.stringify(m)));

$ cat arr.js
module.exports = [3, [2], 1];

Expected:

$ node dyn-import-arr.js 
{"default":[3,[2],1]}

Actual:

$ rollup dyn-import-arr.js -p node-resolve -p commonjs --silent -d dist -f cjs --exports auto && node dist/dyn-import-arr.js
{"0":3,"1":[2],"2":1,"default":[3,[2],1]}

fwiw, esbuild 0.13.8 also differs from NodeJS behavior:

$ esbuild dyn-import-arr.js --bundle | node
{"0":3,"1":[2],"2":1,"default":[3,[2],1]}

/cc @evanw

[guybedford]

Thanks for the clarification - you mean it reads it off the default export? I agree with @kzc that ensuring it's reading the right things is important. Arrays should probably also be filtered too eg module.exports = [....1000 entries...] as a data module should not cause 1000s of iterations.

That does mean something more like typeof m === 'function' || typeof m === 'object' && m && !Array.isArray(m) though.

[lukastaegert]

I extended the code to also ignore arrays

[kzc]

Arrays should probably also be filtered too eg module.exports = [....1000 entries...] as a data module should not cause 1000s of iterations.

Due to the exclusion of the array or string value, iteration over their (own) properties would be avoided.

I extended the code to also ignore arrays

lgtm

Values like non-zero numeric literals would get through that check, but because they don't have any of their own properties it's harmless.