WIP - Add an eslint rule for detecting package level side effects

[talldan]

Fixes #13910

Description

This checks for violations of the package.json sideEffects property. At the moment there are a couple of particular patterns it checks for:

• Function calls in the top-most scope where the return value isn't used. e.g.:

registerStore( // ... );
window.addEventListener( // ... );

• Imports of other modules that don't create any new variables. e.g.:

import './store';

When it detects these patterns and the file has not been added to the package.json sideEffects property it warns the developer to do so.

Other patterns that I've thought about:

• Mutation of an imported value. I think this is possible to add a lint rule for. I imagine there aren't (m)any instances of this, since I'd expect it to be considered bad practice and caught in a code review.
• Any others?

Patterns that are hard to lint for:

• If a mutation or side-effect happens as the result of a function call - e.g. function initializeModule() { window.addEventListener( // ... ); }, it's hard to catch since I'd have to trace back to the callsite.

How has this been tested?

Screenshots

Types of changes

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.

[noisysocks]

Very interesting!! 🤯

[noisysocks]

This is the same example as above.

[talldan]

Yep, was trying to illustrate that adding the file to the sideEffects property resolves the issue, but I need to make it clearer. Will rewrite.

[gziolo]

It might be helpful to have this rule to catch which packages have side-effects but I don't expect this would be bullet-proof. I think we have maybe a handful of packages which don't have side-effects and they are very low-level like:

• autop
• block-serialization-default-parser
• block-serialization-spec-parser
• compose
• deprecated
• element
• escape-html
• is-shallow-equal
• keycodes
• priority-queue
• redux-routine
• tokens-list
• url
• wordcount

and maybe a few more which I didn't explore in depth. For sure this PR contains a few false positives like data or api-fetch which run some logic when imported.

[talldan]

@gziolo Thanks for taking a look. Do you have an example of where data and api-fetch introduce side effects?

I'm looking for other patterns that could be added to this rule, but as you say it'd be hard to make anything bullet-proof.

I think if a rule can help manage those side-effects it'd still be beneficial to making it a completely manual process, and should also help alert devs to the need to think about side effects.

[gziolo]

@gziolo Thanks for taking a look. Do you have an example of where data and api-fetch introduce side effects?

Sure, a related line for data:

• https://github.com/WordPress/gutenberg/blob/master/packages/data/src/default-registry.js#L6

I might be wrong about api-fetch - it probably sets up all middlewares on the server in WordPress core.

[talldan]

Closing this as it has lain dormant for some time and there's a bit more thought that needs to go into it.

[gziolo]

Closing this as it has lain dormant for some time and there's a bit more thought that needs to go into it.

Do you still think, we should we at least pick some of the packages manually and apply this magic field to those which are all pure?