fix(commonjs): support CJS modules re-exporting transpiled ESM modules

[fwouts]

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

List any relevant issue numbers:
#1116
vitejs/vite#2139

Description

Rollup with the CommonJS plugin currently cannot handle the following scenario (real example with next@12.1.4):

Example ESM code:

import Link from 'next/link`  // default import

Definitions in next@12.1.4 NPM package (CJS):

// next/link.js
module.exports = require('./dist/client/link');

// next/dist/client/link.js
Object.defineProperty(exports, "__esModule", {
    value: true
});

const Link = // ...

exports.default = Link;

This happens because the __esModule property is not defined in next/link.js, but inside next/dist/client/link.js instead. Technically, next/link.js does have this property transitively, but that is not apparent with static analysis.

In order for this to work, we need to check the value __esModule at runtime (unless we prefer to analyze files recursively with Rollup, which appears difficult given Rollup's architecture). For that to happen, shouldWrap and detectWrappedDefault must both be true.

This PR implements this additional check. As a side-effect, one existing snapshot for produces optimized code when importing esm with a known default export is arguably not as optimized anymore. I'm not sure it's possible to fix this bug while keeping that particular example as simple, which means some Rollup builds may end up slightly larger. I'd argue this is better than Rollup incorrectly wrapping default exports!

Note 1: This PR should merge relatively cleanly with #1038. I'm happy to submit a patch for that branch as well.

Note 2: I am new to this codebase, and I could obviously have missed an important use case. I'd love a bit of guidance to make sure it's top-notch before it gets merged.

@lukastaegert FYI

[lukastaegert]

I think it looks good and is rather minimal.

Two things:

• Could you convert the tests as I outlined?
• Could you rebase to and change your PR target directly to the commonjs/strict-require-order branch? Then I will make sure they are released together and I do not need to resolve the merge conflicts :) I really want to release that thing now.

[lukastaegert]

Could you do me a favour and instead of adding to this huge test file (which I would really hope to get rid of eventually, but we need it for some tricky caching things) and instead convert them to "function" tests? The reason is that for those tests, everything is neatly together in a separate folder and you do not have to scroll and search around for your fixtures.
Basically for each test, you add a folder to test/fixtures/function with your test fixtures (entry should always be main.js) and a _config.js file as well with something like

module.exports: {
  description: '...some text to describe the test case',
  // options for the commonjs plugin
  pluginOptions: {
    defaultIsModuleExports: 'auto'
  }
}

[fwouts]

Of course, will do in the next day or two! Thanks for the speedy review :)

[fwouts]

Note this particular bug that the change introduces. I'm not sure it's possible to avoid it, but would love suggestions.

[lukastaegert]

Hmm, will need to thing about this. In any case your observation

In order for this to work, we need to check the value __esModule at runtime (unless we prefer to analyze files recursively with Rollup, which appears difficult given Rollup's architecture).

is not correct. Rollup can "look into a module before including it" vie this.load, and as a matter of fact, it already does extensively in resolve-require-sources. So it should actually be possible to extend this logic to figure out e.g. if we want to create a default export I guess (currently we only know if we are requiring CJS or ES), but this will make things MUCH more complicated.

[fwouts]

Ah! That's useful info. I'd be willing to explore that if you prefer.

[lukastaegert]

Well, have a look, but the logic is slightly involved, feel free to ask questions. I guess the current logic will be good enough as well, but I will need to think a little more about it.

[fwouts]

Thank you, still exploring and not quite understanding everything yet but I do have a question!

Is the definition of this test intentionally replacing the module.exports object—which has its __esModule property set to true—with a string? Is the idea to have the static analysis kick in but have __esModule missing at runtime?

[fwouts]

After exploring a bit more, I found out that this spurious default export is already expected in other tests such as this one, so it's not actually new behaviour, and not something we need to worry about in the context of this PR.

I think there is a more general refactor required, which like you suggested would look at the meta data collected in required modules and only generate a default export if we expect there to be one. This could even allow us to remove the getDefaultExportFromCjs helper entirely (it would always be export default x["default"]).

I'm now of the opinion that this general refactor would be better done in a separate PR, to be implemented after #1038. It may also be alright to leave it unchanged :)

I propose to merge this PR as-is.

[fwouts]

Ready for another round of review!

[lukastaegert]

Looks good to me! Will look into getting this one released.