Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(commonjs)!: default strictRequires to true #1639

Merged
merged 15 commits into from
Sep 23, 2024

Conversation

bluwy
Copy link
Contributor

@bluwy bluwy commented Nov 27, 2023

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

List any relevant issue numbers: #1425

resolves #1425

additional discussion: #1618 (review)

Description

Changes the strictRequires option default from "auto" to true, based on the suggestion at #1425 (comment). 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
Copy link
Collaborator

@bluwy checking in on this one

@bluwy
Copy link
Contributor Author

bluwy commented Jan 10, 2024

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.

@bluwy bluwy marked this pull request as ready for review January 10, 2024 05:35
@bluwy bluwy requested a review from shellscape as a code owner January 10, 2024 05:35
@lukastaegert
Copy link
Member

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 lukastaegert force-pushed the commonjs-strict-requires-true branch 2 times, most recently from 2c9254b to 62bdd49 Compare January 18, 2024 05:37
@lukastaegert lukastaegert force-pushed the commonjs-strict-requires-true branch from 7aa06b2 to dc6a4d8 Compare January 19, 2024 09:04
@lukastaegert
Copy link
Member

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)

@lukastaegert lukastaegert force-pushed the commonjs-strict-requires-true branch from 4f89682 to 3d78403 Compare January 19, 2024 15:32
@bluwy
Copy link
Contributor Author

bluwy commented Jan 21, 2024

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.

@shellscape
Copy link
Collaborator

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

@bluwy
Copy link
Contributor Author

bluwy commented Sep 23, 2024

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

@shellscape
Copy link
Collaborator

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

@bluwy
Copy link
Contributor Author

bluwy commented Sep 23, 2024

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.

@shellscape
Copy link
Collaborator

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

@shellscape shellscape merged commit 8f02987 into rollup:master Sep 23, 2024
5 checks passed
@shellscape
Copy link
Collaborator

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.

@bluwy
Copy link
Contributor Author

bluwy commented Sep 23, 2024

Thanks for handling it!

@bluwy bluwy deleted the commonjs-strict-requires-true branch September 23, 2024 16:47
@shellscape
Copy link
Collaborator

No worries. Appreciate the patience around these PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rollup/commonjs plugin sometimes fails to detect circular dependency
3 participants