feat(commonjs)!: default strictRequires to true

[bluwy]

Rollup Plugin Name: commonjs

This PR contains:

• [ ] bugfix
• [x] feature
• [ ] refactor
• [ ] documentation
• [ ] other

Are tests included?

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

Breaking Changes?

• [x] 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. (NOTE: I've forgot to do this in my first commit)

List any relevant issue numbers: #1425

resolves #1425

additional discussion: https://github.com/rollup/plugins/pull/1618#pullrequestreview-1737314762

Description

Changes the strictRequires option default from "auto" to true, based on the suggestion at https://github.com/rollup/plugins/issues/1425#issuecomment-1465626736. This helps resolve race conditions that causes circular import detection to fail, and helps ensure the build hash is consistent.

Commit explanation:

1. Change default
2. Update fixtures
3. Update some test values <---- (CI should still fail)
4. Update remaining fails to use "auto" instead <---- (CI should pass here)

I've committed 3rd and 4th separately to view what 4th had fixed. I'd like an eye on the fails in the 3rd commit because I think it revealed some issues with defaulting to true now.

IMO the fact that true and "auto" have different exported outputs makes me a bit worried of this change. Or maybe I'm missing something.

[shellscape]

@bluwy checking in on this one

[bluwy]

There's not much left on my end, I'm waiting for feedback on this:

I've committed 3rd and 4th separately to view what 4th had fixed. I'd like an eye on the fails in the 3rd commit because I think it revealed some issues with defaulting to true now.

[lukastaegert]

I will have a look. So far, I already identified one issue with handling moduleSideEffects: In a wrapped module, it does not have any effect due to the wrapping. Instead, we need to precede every call to the require function with a pure comment if it would be false. I will see if I can make it work. Proxies on the other hand would need to have their moduleSideEffects mirror the moduleSideEffects of the proxied module, or better with the above change, always have them set to true.

[lukastaegert]

Hi @bluwy. I looked at all test problems and there were indeed a few things I found. I hope I sufficiently addressed all of them:

• Some tests just needed small updates in their assertions but were essentially fine
• defaultIsModuleExports: false was not handled correctly for wrapped modules, which I fixed
• I extended the readme to explain that at the moment, CommonJS entry points will only have a default export, and what to do about it
• moduleSideEffects were not handled correctly for wrapped modules. The solution is now that whenever we have moduleSideEffects:false for a wrapped module, all transformed requires of that module will be preceded with an @__PURE__ annotation, which should have the same effect (i.e. side effects only matter if the required exports are used)

[bluwy]

Thanks! I'm not too familiar with the commonjs plugin internals, so I appreciate your help taking a look 🙏 The commit messages were helpful and the changes make sense to me.

I don't have any more changes from my end, so with your changes in I think the PR should be good to go now.