fix(commonjs): avoid wrapping commonjsRegister call in createCommonjsModule(...)

[danielgindi]

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

Description

In cases where the code has an strong indicator for wrapping, like /__esModule/.test(code), the transform forces the code to be wrapped in a createCommonjsModule.
This is redundant when the code is already a commonjsRegister call, as it registers a module with full commonjs mechanics.

(Of course the specific case outlined with /__esModule/ is not the best and there's a // TODO handle transpiled modules in the code over there.
But there are multiple indicators for wrapping, and this could be avoided. It's just extra mess that does not hurt, but does not contribute anything.
The result of the wrapped module is not used anywhere, it just calls the commonjsRegister inside the createCommonjsModule.

[danielgindi]

@shellscape @lukastaegert
This is a cleanup that will immediately result in less complexity and less code in some setups (using dynamic requires).
This was also tested in a very complex configuration.
IMHO it's safe to merge.

[lukastaegert]

Unfortunately I do not think we can keep it this way, see my comment. The resulting code would only work for cjs targets, and I am not sure there is a simple fix for other targets that does not involve adding exceptions everywhere. My feeling is that for now, we would be happier with keeping the wrapping in this case. Also note that larger changes here will present terrible merge conflicts with #537 once this moves forward again (I actually consider picking this up myself as it might have a huge impact).

[lukastaegert]

If you run this test but add a parameter format: "es" here and console.log what the code actually is, you will see that it is invalid as tries to assign something to a non-existing (in an ES module) module.exports, which would throw at runtime.

Not sure we can keep it as it is. The only way to fix this is to get rid of all exports, which seems rather complex for me considering it is not entirely sure how this feature will evolve.

[danielgindi]

I logged the test result, and it does not seem to generate anything invalid.
It's assigning to module.exports while module is valid and passed as an argument from commonjsRegister's callback.

[lukastaegert]

Ah, sorry, did not read your reply properly, will need to have another look again myself

[lukastaegert]

Oh you are right and I stand corrected. In that case there is no issue from my side.

[danielgindi]

Unfortunately I do not think we can keep it this way, see my comment. The resulting code would only work for cjs targets, and I am not sure there is a simple fix for other targets that does not involve adding exceptions everywhere. My feeling is that for now, we would be happier with keeping the wrapping in this case. Also note that larger changes here will present terrible merge conflicts with #537 once this moves forward again (I actually consider picking this up myself as it might have a huge impact).

I'm fine with putting this on hold, if there's work to resolve that // TODO handle transpiled modules - it should fix most of the cases that this issue appears.
Anyway I'll also dive in again to see the issues you pointed at, it may be simpler than that.

[shellscape]

@danielgindi I merged your previous PR and it caused a few conflicts. Please have a look and we'll merge

[danielgindi]

@shellscape Rebased