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

Mark all generated package.json files as "sideEffects": false. #8213

Merged
merged 2 commits into from
May 12, 2021

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented May 12, 2021

Should fix #8168 and #5686.

The only side-effectful file we previously advertised was the fixPolyfills.native.js module within @apollo/client/cache, but that file should be used only in React Native, whose Metro bundler does not perform tree-shaking or dead code elimination, which means we never really needed to defend that file from tree-shaking in the first place. In other words, I believe we can get away with putting "sideEffects": false in all of our package.json files, for simplicity (as correctly conjectured by @nwalters512 in #8168).

Background explanation of the "sideEffects": false convention: https://webpack.js.org/guides/tree-shaking/#mark-the-file-as-side-effect-free

@benjamn
Copy link
Member Author

benjamn commented May 12, 2021

@hwillson @brainkim I think this PR is safe for v3.3.x, but I will cut a v3.4.0-beta.x release first, so we/folks can verify the benefits before we release v3.3.18.

@benjamn benjamn linked an issue May 12, 2021 that may be closed by this pull request
Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

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

👍 thanks @benjamn!

Should fix #8168.

The only side-effectful file we previously advertised was the
fixPolyfills.native.js module within @apollo/client/cache, but that file
should be used only in React Native, whose Metro bundler does not
perform tree-shaking or dead code elimination. In other words, I believe
we can get away with "sideEffects": false in all of our package.json
files, for simplicity.
@benjamn benjamn force-pushed the reinstate-sideEffects-false-in-package.json-files branch from 4a36514 to 31636dc Compare May 12, 2021 23:37
@benjamn benjamn merged commit cdb683c into main May 12, 2021
@benjamn benjamn deleted the reinstate-sideEffects-false-in-package.json-files branch May 12, 2021 23:41
@phbou72
Copy link

phbou72 commented May 17, 2021

Hi! I'm not sure if this is normal or what in our setup is broken but I'm getting this error when building with Webpack when using that patch :

[client (latest)] Compiling
[error]

[client (latest)] Compile failed with errors
Error: webpack has removed these modules:
    "./node_modules/@apollo/client/cache/index.js",
    "./node_modules/@apollo/client/cache/inmemory/fixPolyfills.js",
    "./node_modules/@apollo/client/index.js"

• If a module is necessary for production, preserve it by adding it to package.json#sideEffects (for more information, see https://github.com/webpack/webpack/tree/master/examples/side-effects)
• If a module can be safely removed, add it to FailOnUnexpectedModuleShakingPlugin's 'exclude' list
    at SOURCE_PATH...

I tried adding the modules to the sideEffects field but no luck!

@nwalters512
Copy link

What is FailOnUnexpectedModuleShakingPlugin? I can't find info or source code for that online. Is that a custom in-house plugin you're using? Did you try the suggestion to add the files to the plugin's exclude list?

@brainkim
Copy link
Contributor

@phbou72 As far as I can tell, that is a custom Shopify Webpack plugin? Can you provide more information?

@phbou72
Copy link

phbou72 commented May 18, 2021

Thanks for giving feedback so fast guys. I've clarified the situation with the guys at Shopify and yeah like you said it was a plugin causing this. So it's all resolved now! Thanks!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nested package.json files break tree shaking Issues with tree-shaking in v3 beta
5 participants