fix(commonjs): replace top-level this with exports name#1618
fix(commonjs): replace top-level this with exports name#1618shellscape merged 2 commits intorollup:masterfrom
Conversation
|
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. |
|
Thanks. If we do a breaking change, perhaps we can also sneak #1425 (comment) (changes |
lukastaegert
left a comment
There was a problem hiding this comment.
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.
|
@bluwy can we get that |
|
Hey, sorry for the silence here. I was actually working on a PR to change the 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. |
|
@bluwy looks like we've a conflict here too |
Rollup Plugin Name:
commonjsThis PR contains:
Are tests included?
Breaking Changes?
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_segmentationreported from the Vite issue above.The issue is that the library ships with this code:
Where
thisis used instead ofmodule.exports, but in practice they're equal (stackblitz example). However,@rollup/plugin-commonjsreplacesthisascommonjsGlobalso it points to globalThis/window/global/self which is incorrect.This PR updates to replace
thisto theexportsName, which should work likemodule.exports. I also updated the test that was testing the previous behaviour.