@bluwy checking in on this one

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.

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.

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)

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.

@bluwy would you be up for correcting the conflict with that test snapshot?

I updated the branch which updates /rollup-plugins/packages/commonjs/test/snapshots/function.js.md and the .snap files.

@bluwy thanks mate. unfortunately your other PR #1618 that I just merged caused more conflicts 😭

No problem 👍 Just updated it again. About #1618, I think we discussed that it's technically a bug fix but could be breaking enough that it would be safer in a major, but I noticed it's already automatically released as a patch.

Just flagging, not sure if it's something we need to concern about.

Damn. This is why we have several indicators. I missed that part of the conversation.

fwiw I reverted that, and then cherry-picked the commit onto master, and then merged this. I'll manually update the changelog after this is released.

Thanks for handling it!

No worries. Appreciate the patience around these PRs.