-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Rewrite sideEffects flags to use only positive patterns #26452
Conversation
This PR is something that @sgomes is the most suited person to review :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @jsnajdr! These changes look good to me.
I added some comments around problematic or confusing usage of side effects, not so much as something to be fixed in this PR (that would be way out of scope), but rather as a list of things that made this PR harder. Hopefully they can be fixed over time.
Before merging this we'd ideally do some testing with Gutenboarding or another consumer that doesn't make full use of everything, to see if the resulting bundle sizes don't change too much. In practice, that's a bit hard to do (at least with Gutenboarding), so we'll probably just have to push these changes live and see how they work. They seem safe in that they shouldn't break anything, but they may result in larger bundles for consumers.
@@ -23,10 +23,6 @@ | |||
"main": "build/index.js", | |||
"module": "build-module/index.js", | |||
"react-native": "src/index", | |||
"sideEffects": [ |
There was a problem hiding this comment.
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.
@@ -21,10 +21,6 @@ | |||
"main": "build/index.js", | |||
"module": "build-module/index.js", | |||
"react-native": "src/index", | |||
"sideEffects": [ |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 🙂
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@@ -22,10 +22,6 @@ | |||
"main": "build/index.js", | |||
"module": "build-module/index.js", | |||
"react-native": "src/index", | |||
"sideEffects": [ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 Slot
s are still there.
There was a problem hiding this comment.
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 👍
@@ -22,10 +22,6 @@ | |||
"main": "build/index.js", | |||
"module": "build-module/index.js", | |||
"react-native": "src/index", | |||
"sideEffects": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above.
@@ -22,10 +22,6 @@ | |||
"main": "build/index.js", | |||
"module": "build-module/index.js", | |||
"react-native": "src/index", | |||
"sideEffects": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above.
a5bd343
to
b80f0c3
Compare
Size Change: +66 B (0%) Total Size: 1.2 MB
ℹ️ View Unchanged
|
@sgomes I added some missing declarations for The rules for styles are:
@gziolo I have a local Node script that goes through the |
Thanks, @jsnajdr! I've taken a look, and everything looks correct to me, according to the rules you've described 👍 We could probably go as far as proactively adding the style declarations to every package, regardless of whether they have any styles, but you never know if that might cause a particular bundler or bundler version to break. Your linter approach is definitely safer 👍 |
Since v5, webpack doesn't support extended globs in the
sideEffects
flag, which also include the!
operator to negate glob conditions. We therefore need to rewrite our patterns to be positive 🙂There are several categories of packages and side effects in the Gutenberg repo, and the set of used combinations is rather small:
some packages are not libraries that would have many exports to use by consumers, but are apps or plugins that have one entrypoint function (usually called like
initialize()
). They don't need to declare any side effects, as it's expected to bundle and use the entire package. Examples areedit-post
,edit-site
,edit-navigation
,edit-widgets
. In the Gutenberg plugin, they are used by PHP pages that load the script and then call, e.g.,window.wp.editWidgets.initialize()
. Then there's theblock-directory
plugin that is initialized simply by loading the script. From these packages, I removed the existingsideEffects
declarations frompackage.json
. The flag defaults tosideEffects: true
, as desired.some packages register a data store. Then the files that have side effects are
store/index.js
(contains theregisterStore
call) andindex.js
(contains theimport './store'
call, which is itself a side effect that needs to be declared)some packages install hooks (actions or filters). That usualy adds
hooks/**
as side-effectful, and also the rootindex.js
that doesimport './hooks'
some packages have
*.scss
files in their sources and produce abuild-style
directory. This directory needs to be declared as side-effectful.I'm not sure whether we need to declare
src/**/*.scss
, too. These files are every directly imported only by the React Native code. I don't know how that is bundled.Todo:
build-style
and do they need them declared as side-effectful?This PR is a spinoff from the big webpack 5 upgrade branch in #26382.