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

fix(commonjs): replace top-level this with exports name #1618

Merged
merged 2 commits into from
Sep 23, 2024

Conversation

bluwy
Copy link
Contributor

@bluwy bluwy commented Oct 29, 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.

List any relevant issue numbers: vitejs/vite#4083 (comment)

EDIT: I guess it could be considered a breaking change is someone relied on the bug, but I usually still think of it as a bug fix.

Description

This fixes compatibility with @mediapipe/selfie_segmentation reported from the Vite issue above.

The issue is that the library ships with this code:

;(() => {
  this.namedExports = 'foo'
}).call(this)

Where this is used instead of module.exports, but in practice they're equal (stackblitz example). However, @rollup/plugin-commonjs replaces this as commonjsGlobal so it points to globalThis/window/global/self which is incorrect.

This PR updates to replace this to the exportsName, which should work like module.exports. I also updated the test that was testing the previous behaviour.

@shellscape
Copy link
Collaborator

Thanks for the PR. This looks like a breaking change since the output will change significantly. I would like to get eyes on this from @guybedford and @lukastaegert before we merge.

@bluwy
Copy link
Contributor Author

bluwy commented Oct 30, 2023

Thanks. If we do a breaking change, perhaps we can also sneak #1425 (comment) (changes strictRequires to true by default). I was also checking the commonjs issue yesterday.

Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good to me. I agree that the change is a bug fix as the default behavior in Node is indeed that this is NOT the global object. But it is indeed possible to rely on the wrong behavior, so I fear we need to make it breaking. And if we do that, I agree that we should use the chance to change strictRequires: true to not spam major versions.

@shellscape
Copy link
Collaborator

@bluwy can we get that strictRequires change into here as well?

@bluwy
Copy link
Contributor Author

bluwy commented Nov 27, 2023

Hey, sorry for the silence here. I was actually working on a PR to change the strictRequires default, but that made a lot of tests failed. Besides updating the fixtures, seems like some tests are actual fails that expects the auto option, which seems to fix things that true didn't (?).

The changes are quite large so it's probably better as a separate PR. I'll submit it shortly and we could see how to work on it from there.

@shellscape
Copy link
Collaborator

@bluwy looks like we've a conflict here too

@shellscape shellscape merged commit 2fdd077 into rollup:master Sep 23, 2024
5 checks passed
@bluwy bluwy deleted the fix-commonjs-replaced-this branch September 23, 2024 15:07
shellscape added a commit that referenced this pull request Sep 23, 2024
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.

3 participants