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

Add sideEffects property to package.json for our packages #13910

Closed
talldan opened this issue Feb 18, 2019 · 3 comments
Closed

Add sideEffects property to package.json for our packages #13910

talldan opened this issue Feb 18, 2019 · 3 comments
Labels
npm Packages Related to npm packages [Status] In Progress Tracking issues with work in progress [Type] Build Tooling Issues or PRs related to build tooling

Comments

@talldan
Copy link
Contributor

talldan commented Feb 18, 2019

Description
The sideEffects property indicates whether a package introduces any side effects. It can be specified as either false or as an array of files that have side effects.

By specifying this property, webpack is able to reduce the size of JavaScript bundles by eliminating unused imports:
https://webpack.js.org/guides/tree-shaking/

sideEffects is particularly relevant when implementing feature flags. Without it, features need to be flagged at every possible point. For further details, see this discussion: #13829 (comment)

How to implement this
Packages will need to be assessed individually. The biggest cause of side effects is likely to be registerStore. Any function call in the outer-most scope can result in a side-effect, particularly if there's no return value:

registerStore( //... );
registerBlock( // ... );

Mutation of global or imported variables is another example:

import store from '@wordpress/core-data';

modifyStore( store );
store.foo = bar;

window.shim = window.shim || function() {
    // ...
};

Another good indication is any import that doesn't define any new variables. The imported file will almost certainly have side-effects:

import 'something-with-side-effects';

Other ideas
It might be possible to use a linting rule to enforce this. A quick look at npm shows limited prior art, so a custom rule might be needed.

@talldan talldan added [Type] Build Tooling Issues or PRs related to build tooling [Type] Code Quality Issues or PRs that relate to code quality npm Packages Related to npm packages and removed [Type] Code Quality Issues or PRs that relate to code quality labels Feb 18, 2019
@talldan talldan self-assigned this Feb 27, 2019
@gziolo gziolo added the [Status] In Progress Tracking issues with work in progress label Apr 23, 2019
@gziolo
Copy link
Member

gziolo commented Apr 23, 2019

@talldan should we enable sideEffects property in some of the packages manually as the first step?

@gziolo
Copy link
Member

gziolo commented May 9, 2020

@talldan do you plan to continue work on this issue? We added this flag to many packages where it was straightforward. I would be fine with closing this issue if handling the rest is too complicated and thus unreliable in the long run.

@talldan talldan removed their assignment May 13, 2020
@talldan
Copy link
Contributor Author

talldan commented May 13, 2020

@gziolo Sorry for missing your pings. I didn't realise I'd self-assigned this. I stopped working on the eslint rule for package side effects (#14135)—although it half-worked there were too types of side-effects it didn't catch. I should've unassigned myself when closing that.

I think you're right, having an open issue here doesn't add much value. I think it's fine to continue on a case by case basis for packages and close this. Thanks for handling those PRs 👍 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
npm Packages Related to npm packages [Status] In Progress Tracking issues with work in progress [Type] Build Tooling Issues or PRs related to build tooling
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants