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.

Sign up for GitHub

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

Rewrite sideEffects flags to use only positive patterns #26452

Merged
merged 2 commits into from
Oct 27, 2020
Merged

Rewrite sideEffects flags to use only positive patterns #26452

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1ceb802
Rewrite sideEffects flags to use only positive patterns
jsnajdr Oct 22, 2020
b80f0c3
Add sideEffects declarations for src/**/*.scss files
jsnajdr Oct 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 0 additions & 13 deletions packages/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,17 +308,4 @@ If your package includes a few files with side effects, you can list them instea
}
```

Many `@wordpress` UI-focused packages rely on side effects for registering blocks, plugins, and data stores. To reduce maintenance costs, it may be preferable to opt for an inverse glob strategy, where you instead list the paths where side effects are *not* present, leaving the bundler to assume that everything else might have them. This results in a glob with multiple roots (to match `@wordpress` package structure) and one or more excluded directories.

Here is an example where we declare that the `components` and `utils` directories are side effect-free:

```json
{
"name": "package",
"sideEffects": [
"!((src|build|build-module)/(components|utils)/**)"
],
}
```

Please consult the [side effects documentation](./side-effects.md) for more information on identifying and declaring side effects.
4 changes: 0 additions & 4 deletions packages/block-directory/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,6 @@
"main": "build/index.js",
"module": "build-module/index.js",
"react-native": "src/index",
"sideEffects": [
Copy link
Contributor

Choose a reason for hiding this comment

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

This package confuses me. How is it supposed to be used?

Is it meant to be a single side effect, by doing just import '@wordpress/block-directory';? I'm seeing a bunch of components in there that don't seem to be loaded directly, but I guess they're all loaded transitively?

Copy link
Member Author

Choose a reason for hiding this comment

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

block-directory is a JS plugin. You just load the script and it hooks into the wp.data, wp.hooks and wp.plugins APIs, adding a new feature to the editor.

If you were building your own app with the NPM packages, the package would be activated by a simple side-effectful import, as you wrote. The APIs it hooks into are (ideally) peer dependencies of the block-editor package.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm seeing a bunch of components in there that don't seem to be loaded directly, but I guess they're all loaded transitively?

These are probably all used to implement the plugin UI, rather than being exported for 3rd party consumers. Are there any specific components that look suspicious?

Copy link
Contributor

Choose a reason for hiding this comment

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

None in particular. I expect them all to be used to implement the plugin UI, yes, unless one of them got dropped over time.

Copy link
Member

Choose a reason for hiding this comment

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

We could make it side effects free. I didn't consider it when refactoring to behave like a plugin. Edit family of packages uses initialize methods and we could offer something similar. Definitely a follow up task if you feel like it's worth the hassle.

Would it be useful if more functionality would be initialized this way?

Copy link
Contributor

@sgomes sgomes Oct 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Great question, @gziolo!

In general, I feel that any package that could be classified as a library/plugin/utility (that is, something that doesn't invert control) should avoid side effects as much as possible.

Even setting aside the issues with bundlers for a second, initialisation functions generally provide more flexibility, because the consumer has more control over when they're invoked. Consumers can e.g. import something statically and call initialize() only when the editor is being loaded; without an initialisation method, they'd have to instead use a dynamic import, which by its async nature could be hard to fit into an existing codebase.

In terms of difficulty, it's generally a trivial amount of extra work for the naive use-case anyway. It's really not too bad to go from:

import 'side-effectful-module';

to

import { initialize } from 'side-effect-free-module';
initialize();

This way, users who don't care still get their initialisation at the top level of their module, while users who do care can move it elsewhere.

And on the package side, it reduces guesswork and maintenance around the sideEffects property in package.json, as this PR sadly shows 😕

So while I don't think it's anything urgent that needs immediate attention, I'd definitely recommend switching to initialisation methods whenever the next code revamp opportunity comes along 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

Would it be useful if more functionality would be initialized this way?

What @sgomes said 🙂 I'll only add that the inflexible initialization was sometimes causing problems in the first Gutenlypso integration 2 years ago. The problematic code usually looks like this:

import '@wordpress/rigid';
import flexible from '@wordpress/flexible';

getInitData().then( initData => {
  flexible.init( initData );
  // I'd love to do the following, but can't
  // rigid.init();
} );

The rigid package needs to be initialized only after flexible (because, for example, it does select( 'core/flexible' ) during initialization), but I can't do that. import statements must be at the top level of the module, not inside a callback.

Solutions are either using require( '@wordpress/rigid' ), or creating an extra async-loaded module that is loaded just-in-time.

On the other hand, I think that declaring sideEffects on absolutely everything is not needed. sideEffects: false is useful for libraries where you can use just a little part of the library and want to avoid bundling the rest.

For plugins like block-directory or apps like edit-post we can keep the default where everything is assumed to have side effects, and I don't think it causes much deoptimization if any.

Copy link
Member

Choose a reason for hiding this comment

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

In WordPress core we would have to call wp.blockLibrary.initialize() but it wouldn't be that much work. It's definitely something to keep in mind when working on new packages.

"build-style/**",
"!((src|build|build-module)/components/**)"
],
"dependencies": {
"@wordpress/a11y": "file:../a11y",
"@wordpress/api-fetch": "file:../api-fetch",
Expand Down
3 changes: 2 additions & 1 deletion packages/block-editor/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@
"react-native": "src/index",
"sideEffects": [
"build-style/**",
"!((src|build|build-module)/(components|utils)/**)"
"src/**/*.scss",
"{src,build,build-module}/{index.js,store/index.js,hooks/**}"
],
"dependencies": {
"@babel/runtime": "^7.11.2",
Expand Down
3 changes: 2 additions & 1 deletion packages/block-library/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@
"module": "build-module/index.js",
"react-native": "src/index",
"sideEffects": [
"build-style/**"
"build-style/**",
"src/**/*.scss"
],
"dependencies": {
"@babel/runtime": "^7.11.2",
Expand Down
2 changes: 1 addition & 1 deletion packages/blocks/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
"module": "build-module/index.js",
"react-native": "src/index",
"sideEffects": [
"!((src|build|build-module)/api/**)"
"{src,build,build-module}/{index.js,store/index.js}"
],
"dependencies": {
"@babel/runtime": "^7.11.2",
Expand Down
3 changes: 2 additions & 1 deletion packages/components/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@
"module": "build-module/index.js",
"react-native": "src/index",
"sideEffects": [
"build-style/**"
"build-style/**",
"src/**/*.scss"
],
"dependencies": {
"@babel/runtime": "^7.11.2",
Expand Down
2 changes: 1 addition & 1 deletion packages/core-data/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
"module": "build-module/index.js",
"react-native": "src/index",
"sideEffects": [
"(src|build|build-module)/index.js"
"{src,build,build-module}/index.js"
],
"dependencies": {
"@babel/runtime": "^7.11.2",
Expand Down
4 changes: 0 additions & 4 deletions packages/edit-navigation/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,6 @@
"main": "build/index.js",
"module": "build-module/index.js",
"react-native": "src/index",
"sideEffects": [
Copy link
Contributor

Choose a reason for hiding this comment

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

This module seems to export an initialize method, so I'm wondering why the data setup isn't done through that, rather than as a side effect? Even if everything is indeed needed, it would be cleaner and easier to grok.

"build-style/**",
"!((src|build|build-module)/components/**)"
],
"dependencies": {
"@babel/runtime": "^7.11.2",
"@wordpress/api-fetch": "file:../api-fetch",
Expand Down
4 changes: 0 additions & 4 deletions packages/edit-post/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,6 @@
"main": "build/index.js",
"module": "build-module/index.js",
"react-native": "src/index",
"sideEffects": [
Copy link
Contributor

Choose a reason for hiding this comment

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

This package has a whole bunch of components, and this way none of them are modularised 😞 I think that it loads all of them anyway, so it should be fine, but it's hard to tell. And since there's an initialize method, I believe this could be done without side effects here too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean components like PluginBlockSettingsMenuItem that are reexported by edit-post/src/index.js? These are not really components. It's a plugin API that exposes some areas in UI where you can insert your own UI. More precisely, it's a Fill component that inserts something into a sibling Slot, where the pair is created by a createSlotFill call.

If you don't use these pseudo-components, it doesn't mean they can be removed from the bundle. The corresponding Slots are still there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, you're right, I missed the re-exports at the end of the file, my bad 👍

"build-style/**",
"!((src|build|build-module)/(components|utils)/**)"
],
"dependencies": {
"@babel/runtime": "^7.11.2",
"@wordpress/a11y": "file:../a11y",
Expand Down
4 changes: 0 additions & 4 deletions packages/edit-site/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,6 @@
"main": "build/index.js",
"module": "build-module/index.js",
"react-native": "src/index",
"sideEffects": [
Copy link
Contributor

Choose a reason for hiding this comment

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

See above.

"build-style/**",
"!((src|build|build-module)/components/**)"
],
"dependencies": {
"@babel/runtime": "^7.11.2",
"@wordpress/a11y": "file:../a11y",
Expand Down
4 changes: 0 additions & 4 deletions packages/edit-widgets/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,6 @@
"main": "build/index.js",
"module": "build-module/index.js",
"react-native": "src/index",
"sideEffects": [
Copy link
Contributor

Choose a reason for hiding this comment

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

See above.

"build-style/**",
"!((src|build|build-module)/components/**)"
],
"dependencies": {
"@babel/runtime": "^7.11.2",
"@wordpress/api-fetch": "file:../api-fetch",
Expand Down
3 changes: 2 additions & 1 deletion packages/editor/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@
"react-native": "src/index",
"sideEffects": [
"build-style/**",
"!((src|build|build-module)/(components|utils)/**)"
"src/**/*.scss",
"{src,build,build-module}/{index.js,store/index.js,hooks/**}"
],
"dependencies": {
"@babel/runtime": "^7.11.2",
Expand Down
3 changes: 2 additions & 1 deletion packages/interface/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@
"react-native": "src/index",
"sideEffects": [
"build-style/**",
"!((src|build|build-module)/components/**)"
"src/**/*.scss",
"{src,build,build-module}/{index.js,store/index.js}"
],
"dependencies": {
"@babel/runtime": "^7.11.2",
Expand Down
3 changes: 2 additions & 1 deletion packages/nux/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@
"react-native": "src/index",
"sideEffects": [
"build-style/**",
"!((src|build|build-module)/components/**)"
"src/**/*.scss",
"{src,build,build-module}/{index.js,store/index.js}"
],
"dependencies": {
"@babel/runtime": "^7.11.2",
Expand Down
4 changes: 3 additions & 1 deletion packages/primitives/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@
"main": "build/index.js",
"module": "build-module/index.js",
"react-native": "src/index",
"sideEffects": false,
"sideEffects": [
"src/**/*.scss"
],
"types": "build-types",
"dependencies": {
"@babel/runtime": "^7.11.2",
Expand Down
3 changes: 1 addition & 2 deletions packages/reusable-blocks/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,7 @@
"module": "build-module/index.js",
"react-native": "src/index",
"sideEffects": [
"build-style/**",
"!((src|build|build-module)/(components|utils)/**)"
"{src,build,build-module}/{index.js,store/index.js}"
],
"dependencies": {
"@wordpress/block-editor": "file:../block-editor",
Expand Down
3 changes: 2 additions & 1 deletion packages/rich-text/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@
"module": "build-module/index.js",
"react-native": "src/index",
"sideEffects": [
"!((src|build|build-module)/component/**)"
"src/**/*.scss",
"{src,build,build-module}/{index.js,store/index.js}"
],
"dependencies": {
"@babel/runtime": "^7.11.2",
Expand Down
16 changes: 0 additions & 16 deletions packages/side-effects.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,19 +86,3 @@ If it has a few files with side effects, it can list them:
```

This allows the bundler to assume that only the modules that were declared have side effects, and *nothing else does*. Of course, this means that we need to be careful to include everything that *does* have side effects, or problems can arise in applications that make use of the package.

## The approach in `@wordpress`

In order to reduce maintenance cost and minimize the chance of breakage, we opted for using inverse globs for a number of `@wordpress` packages, where we list the paths that *do not* include side effects, leaving the bundler to assume that everything else does. Here's an example:

```json
{
"sideEffects": [
"!((src|build|build-module)/(components|utils)/**)"
],
}
```

The above means that the bundler should assume that anything outside the `components` and `utils` directories contains side effects, and nothing in those directories does. These directories can be inside of a `src`, `build`, or `build-module` top-level directory in the package, due to the way `@wordpress` packages are built.

This approach should guarantee that everything in `components` and `utils` can be tree-shaken. It will only potentially cause problems if one of the files in there uses side effects, which would be a bad practice for a component or utility file.