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

tree shaking and "sideEffects": false ? #843

Closed
macrozone opened this issue Jan 5, 2021 · 3 comments · Fixed by #870
Closed

tree shaking and "sideEffects": false ? #843

macrozone opened this issue Jan 5, 2021 · 3 comments · Fixed by #870
Assignees
Labels
Type: Feature New features and feature requests
Milestone

Comments

@macrozone
Copy link
Contributor

i use uniforms on https://github.com/react-page/react-page to generate forms and i wanted to re-export the bundled uniforms version (using something like export * from 'uniforms-material').

i noticed that this will bundle everything in, even if you don't use any of the re-exports.

I think this is because uniform packages do not declare "sideEffects": false, see https://webpack.js.org/guides/tree-shaking/

i think its quite safe to set that, altough there might actually be some sideeffects (i remember that uniforms extended simpl-schema automatically in some versions, that would be a side effect. not sure if its still the case).

@radekmie radekmie self-assigned this Jan 5, 2021
@radekmie radekmie added the Type: Feature New features and feature requests label Jan 5, 2021
@radekmie radekmie added this to Needs triage in Open Source (migrated) via automation Jan 5, 2021
@radekmie radekmie moved this from Needs triage to To do in Open Source (migrated) Jan 5, 2021
@radekmie
Copy link
Contributor

radekmie commented Jan 5, 2021

Hi @macrozone. Yes, none of the packages actually declare sideEffects: false. I also thought about it some time ago, and, as you said, it'd be fine for most packages. There are three exceptions:

The only problem is that I think that almost none of the projects would benefit from that. The majority of projects use AutoForm which will include all other components. At most, one could eliminate AutoForm and one or two components (e.g. AutoFields). Do you think it's worth to do that, having in mind potential problems?

@macrozone
Copy link
Contributor Author

@radekmie the problem that i had is that

if my library A re-exports library B,
any consumer that include something from A will have B automatically fully bundled, even if no re-export of B is used by the consumer, because the bundler cannot tree-shake B away as it might contain side effects.

My workaround was to not re-export uniforms-material directly, but to only export the Components that i needed as exports as lazy-load components (using something like @loadable/component

i think its worth for the different edge cases.

E.g. consider a project using nextjs, where some pages use uniforms and some don't. Maybe in some pages you just import some types (without the special import type) or helper functions. these pages will have the full uniforms bundled into even if they just use some part of it.

On a side note, i think tree shaking allows libraries to use more complex libraries without declaring them as peer-dependencies (i think peer dependencies are often bad)

@radekmie radekmie added this to the v3.2 milestone Feb 3, 2021
@radekmie radekmie moved this from To do to In progress in Open Source (migrated) Feb 10, 2021
radekmie added a commit that referenced this issue Feb 10, 2021
@radekmie radekmie linked a pull request Feb 10, 2021 that will close this issue
Open Source (migrated) automation moved this from In progress to Closed Feb 17, 2021
radekmie added a commit that referenced this issue Feb 17, 2021
@mariusrak
Copy link
Contributor

This change introduced a bug as I am getting error

Invalid definition for name field: "uniforms" is not a supported property

After downgrade to 3.1.0 it work correctly. I am using webpack, babel and babel preset-env to build app.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature New features and feature requests
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants