feat(commonjs): inject __esModule marker into ES namespaces

[lukastaegert]

Rollup Plugin Name: commonjs

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

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

List any relevant issue numbers:
#481 (comment)

Description

This is an attempt at fixing the issue in the linked comment. It also integrates into the requireReturnsDefault option. So the current solution is:

• if we KNOW a file that is imported by a CJS module is an ES module
• and requireReturnsDefault is false
• then in the generated proxy, we inject a new helper getAugmentedNamespace that copied all property definitions from the namespace to a new object
• in contrast to the actual namespace, the new object is not frozen and has Object as prototype as this is what you would get when requiring from a CJS module
• live-bindings are fully supported

The helper looks like this:

function getAugmentedNamespace(n) {
	var a = Object.defineProperty({}, '__esModule', {value: true});
	Object.keys(n).forEach(function (k) {
		var d = Object.getOwnPropertyDescriptor(n, k);
		Object.defineProperty(a, k, d.get ? d : {
			enumerable: true,
			get: function () {
				return n[k];
			}
		});
	});
	return a;
}

We could have made this even simpler via the plural Object.getOwnPropertyDescriptors but to my understanding this is not IE11 compatible.

This applies to external imports as well if the esmExternals option is set. If you do not want this helper injected, you can set requireReturnsDefault: "namespace" to get the old behaviour. Any value other than false will also not inject the helper.

[lukastaegert]

At the moment, I wonder how this should affect external dependencies and interact with requireReturnsDefault. My feeling is that those should be integrated so that it is also possible to turn off this feature as well as make sure external dependencies are properly augmented as well.

[lukastaegert]

@pachuka Could you check of this PR would solve your issue? I know it is not trivial to test a plugin PR right now, but what works for me is

• check out the plugins repo, install pnpm globally
• run pnpm install in the plugins repo
• cd package/commonjs
• npm link (note that I am not using pnpm here!)
• and then go to where your project is and do an npm link @rollup/plugin-commonjs

[pachuka]

@lukastaegert - I can try it tomorrow, thanks for the instructions!

[lukastaegert]

I updated the PR to integrate with the requireReturnsDefault option, see description and included documentation. I also updated the types with some options I had not included there before.

[pachuka]

@lukastaegert 👏 - just tested it out, seems to work great!!

My diff of my index.cjs.js for my library project now has:

Then in my consuming application, I was able to use my library successfully and the SvgIcons that I was importing from the default namespace from @material-ui/icons in my library components loaded properly.

Thank you for taking a look into this!

[lukastaegert]

I'm converting this one back to draft because with rollup/rollup#3760 (comment), I'm actively considering adding the marker in Rollup core because it might solve a few more issues.

[lukastaegert]

@pachuka Could you check if rollup/rollup#3764 would solve this issue as well if you use the currently published version of the commonjs plugin?

[lukastaegert]

Following the discussion in rollup/rollup#3764 I again removed the draft state here and hope this can be merged and released soon.

[lukastaegert]

Any reviewer comments? @shellscape @danielgindi @guybedford

[shellscape]

I've been following updates, I think it's good to go.

[guybedford]

Looks good!

[guybedford]

Perhaps this function could have an explicit opt-out if __esModule is already defined? Would that be a slight perf improvement in some cases?

[lukastaegert]

Definitely, added!

[chengcyber]

hi, @lukastaegert I wonder whether we could have a helper function like this:

export function getExport (n) {
    // if hasDefaultExport, append namedExport
    if (n && Object.prototype.hasOwnProperty.call(n, 'default')) {
        var newObj = {};
        var hasPropertyDescriptor =
          Object.defineProperty && Object.getOwnPropertyDescriptor;
        var o = n['default'];
        for (var key in o) {
          if (Object.prototype.hasOwnProperty.call(o, key)) {
            var desc = hasPropertyDescriptor
              ? Object.getOwnPropertyDescriptor(o, key)
              : null;
            if (desc && (desc.get || desc.set)) {
              Object.defineProperty(o, key, desc);
            } else {
              newObj[key] = o[key];
            }
          }
        }
        return newObj;
    } else {
        // if no default export, assign all exports as default export
        var newObj = {};
        var hasPropertyDescriptor =
          Object.defineProperty && Object.getOwnPropertyDescriptor;
        var o = n;
        for (var key in o) {
          if (Object.prototype.hasOwnProperty.call(o, key)) {
            var desc = hasPropertyDescriptor
              ? Object.getOwnPropertyDescriptor(o, key)
              : null;
            if (desc && (desc.get || desc.set)) {
              Object.defineProperty(o, key, desc);
            } else {
              newObj[key] = o[key];
            }
          }
        }
        newObj.default = o;
        return newObj;
    }
}

It mimic the helper function like babel/ts importHelper function. This 'namespace' helper func aims to solve the interop issue at import side, but solving esm + cjs interop issue at "export" side might be better IMHO.
I would appreciate hearing your thoughts on this

[lukastaegert]

The helper is shared between importers as all of them are using the same proxy module. So in a way it is applied at export side, but only for those importers that are commonjs. That way, there is no interference with ES module importers.

[lukastaegert]

It mimic the helper function like babel/ts importHelper function

It looks very similar but it is not the same and it serves a very different purpose. It just looks similar methods that try to copy a potentially frozen object and add properties tend to look very similar. Here the issue is that I have a perfectly valid namespace because it came from an ES module, but I need to add the __esModule property without modifying it as that is impossible as the namespace is likely frozen.

The issues that the helpers you mention solve are very different, they are about creating a proper namespace from an object where you do not know a priori if it is an ESM namespace or something else.

Most notably, putting the whole object as the default if there is no default does not make sense to me in this situation.

[lukastaegert]

And mutating an existing default export is a nice way of breaking code. Just imagine the default export is an object, well, then it is a very different object after that.

[chengcyber]

Thanks for your explanation. I can get a more clear idea to the problem this pr is solving

[lukastaegert]

I will take over publishing this one