Gutenberg: Packages: Singleton pattern prone to duplicate, distinct module scopes

[aduth]

Discussed today in the Core JavaScript meeting: https://wordpress.slack.com/archives/C5UNMSU4R/p1534251793000100
Originally raised at: https://github.com/Automattic/wp-calypso/pull/26438#issuecomment-411341882

An original issue was observed with @wordpress/data, where multiple registries could be simultaneously present due to independent versioning of packages having different dependencies upon the specific version of @wordpress/data.

Some observations include...

• This is not specific to @wordpress/data, and could affect anything which behaves in a global / singleton fashion: @wordpress/hooks, @wordpress/filters, @wordpress/blocks
• The issue is mitigated but not eliminated by moving away from Lerna independent versioning (toward fixed versioning) because the chance for differing versions still exists

Options for moving forward considered include...

• Avoid "globals" behavior, where a consumer must define and operate with its own registry (specific to @wordpress/data)
  
  
  
  • How does this work in the WordPress context?
  
  
  • Does a default registry still exist? (specific to @wordpress/data)
  
  

cc @jsnajdr @gziolo @adamsilverstein @youknowriad

[coderkevin]

Avoid "globals" behavior, where a consumer must define and operate with its own registry (specific to @wordpress/data)

Why not just always use registry-provider instead the global value? It already works. Also, setting the value of the provider is compatible with @wordpress/data plugins.

[youknowriad]

@coderkevin Yes this is one of the solutions on the table (for the data module at least) but it's not a simple switch:

• How do plugins access the editor's state if it's not a global registry?
• Should we expose registerBlockType (and similar) in the blocks package or should these be moved to WP specific context only?
• What about the current usage of select,dispatch and subscribe in our current packages.

These have solutions (probably injecting these APIs using inline scripts and forbidding their usage in the packages themselves) but it's not smallish :)

[aduth]

Yes, internally the data module operates on single registries, but also defines a "default" registry, which was specifically intended to improve usability by plugins in a WordPress context (by calling wp.data.select etc). It may be that the module should not define this registry and, if one is to still exist in the WordPress context, it must be defined by the WordPress build. Still an open question as to exactly how this looks. Are those methods still available at wp.data.select, or would this break expectations that wp globals mimic the API interface of their npm counterparts?

[coderkevin]

So, if one uses a plugin and sets the value in the provider, wp.data.select will still map to the "default" registry, from what I'm understanding? If that's the case, it seems a bit broken, right? Plugins should be honored by default.

I am a bit biased as I tend to eschew globals whenever possible. But my preference is for reasons such as this, and for testing as well.

[aduth]

So, if one uses a plugin and sets the value in the provider, wp.data.select will still map to the "default" registry, from what I'm understanding?

No, if a developer uses the provider, any descendents using select will leverage the registry specified by the provider. But a provider is not strictly necessary, in which case the default registry will be used. This is what occurs in Gutenberg.

https://github.com/WordPress/gutenberg/blob/ec1fd2149bf42414c741bdecf86e8c7abdedd2c6/packages/data/src/components/with-select/index.js#L124-L131

See also #7453 as an example of providing a custom registry via provider to create an embedded editor experience.

[ockham]

So, if one uses a plugin and sets the value in the provider, wp.data.select will still map to the "default" registry, from what I'm understanding?

No, if a developer uses the provider, any descendents using select will leverage the registry specified by the provider. But a provider is not strictly necessary, in which case the default registry will be used. This is what occurs in Gutenberg.

FWIW, I think this sort of confusion is quite natural with a double-edged (explicit/default) approach like this (RegistryProvider with explicit registry, vs otherwise falling back implicitly present global one). I was somewhat bitten by that while experimenting with https://github.com/Automattic/wp-calypso/pull/26930 where it would use the global rather than the one I was explicitly providing (I think I was using a wrong prop name for a while, so the consumer would use the default registry).

I don't really have a horse in this race, but I think the purported better developer ergonomics that the global/default registry is supposed to give is set off by the potential errors one makes when trying to migrate to the RegistryProvider-provided one. I'd vote to remove the global one.

[sirreal]

Did you consider using peerDependencies for these problematic modules? After repeated issues with @wordpress/data we're moving dependent packages to use it as a peer: https://github.com/Automattic/wp-calypso/pull/39368

Some reading:

• https://classic.yarnpkg.com/en/docs/dependency-types/#toc-peerdependencies
• https://nodejs.org/en/blog/npm/peer-dependencies/

[jsnajdr]

There are many @wordpress/* packages that should always be declared as peerDependencies of the packages that use them.

There are projects like Calypso or Jetpack that depend on many @wordpress/* packages, in order to either build their own block editor out of the NPM-published packages, or to simply build blocks. Upgrading the @wordpress/* packages to a newer version in these repos is always a painful and error-prone nightmare.

Assume, for example, that our project depends on [email protected] and [email protected]. The [email protected] package declares a dependecy on data@^1.2.3. All these are compatible, so we get one copy of block-editor and data.

Now let's upgrade block-editor to 1.3.0. The 1.3.0 version depends on data@^1.3.0. The two versions of data are no longer compatible and there are now two copies of data: one in node_modules/data (1.2.3) and one in node_modules/block-editor/node_modules/data (1.3.0).

Normally, such duplication only inflates the size of your code (you ship one thing twice), but in case of the data package, it completely breaks the app! The block-editor package registers a core/block-editor data store, but it registers it in its own private instance of the data registry. The rest of the app doesn't see the store and all attempts to select( 'core/block-editor' ) will crash.

We'd rather if npm install refused to install this kind of duplicate dependencies and warned us instead. That's what peerDependencies can do. By declaring data@^1.3.0 a peer dependency of [email protected], the block-editor package says: I want to use the data package, but are not providing it myself. I expect it to be a part of the existing environment. This is typical, e.g., for webpack or ESLint plugins. The plugin doesn't install webpack or eslint package (that's someone else's job), but wants to import modules from it. The same scenario plays out with data.

In our example, npm install will discover that data is already installed, but in an incompatible version (the old 1.2.3). It will issue a warning and will not install a duplicate instance. Exactly what we want.

Which packages should be peer dependencies?
Anything that provides some "environment" or "singleton":

• @wordpress/data -- provides a global store registry
• @wordpress/hooks -- provides a global registry of actions and filters
• @wordpress/plugins -- contains a list of registered plugins
• @wordpress/i18n -- contains global locale data that are used all across the app by __()

There are other packages that register a data store and provide various services for the app:

• @wordpress/notices
• @wordpress/viewport
• @wordpress/keyboard-shortcuts

If these packages are NPM dependencies anywhere, they should be also peers. But the consumers don't usually import anything from these packages -- they are accessed through the registered data store, e.g.:

dispatch( 'core/notices' ).createNotice( ... );

and not with:

import { createNotice } from '@wordpress/notices';
createNotice( ... );

What if we removed the singletons, e.g., defaultRegistry, and used React context providers?
That's nice and desirable, but @wordpress/data still needs to be a peer dependency even without defaultRegistry. Because the React context itself (the thing returned from React.createContext()) is a singleton and should have only one instance.

If the app (e.g., edit-post) renders a context provider at the top:

import { DataProvider } from '@wordpress/data';
ReactDom.render( <DataProvider registry={ registry }><App /></DataProvider> );

and a component uses the context consumer:

import { DataConsumer } from '@wordpress/data';
export default function Component() {
  return <DataConsumer>
    ( registry ) => { ... }
  </DataConsumer>
}

then the provider and consumer must come from the same package. If the consumer imports from a duplicate, then it's two different contexts (two different results of React.createContext() call) and the provider and consumer will not see each other!

The same reasoning applies for @wordpress/i18n, too.

[jsnajdr]

Another case where a dependency should always be a peer is when the consumer imports a Fill component from a slot/fill pair created by a createSlotFill pair.

One example is the block-directory plugin that imports PluginPrePublishPanel from @wordpress/edit-post. It plugs in some UI into one of the editor panels, and edit-post is clearly a part of the surrounding environment. The edit-post has approx 10 slots like this that can be filled by plugins.

[jsnajdr]

There are other packages that register a data store and provide various services for the app

For the record, there is a convention that a package should declare another package as a NPM dependency even if it merely uses a store registered by it, without doing any import. @youknowriad did a PR that added some missing ones: #23517.

[gziolo]

With npm v7 peerDependencies change behavior and they trigger package installation. Once we migrate Gutenberg to the new version, it should be all straightforward to update some of the dependencies @jsnajdr mentioned to be defined as peer dependencies.

It's a bit unclear if Lerna would be capable of handling them as expected. To better illustrate it let's take an example:
https://github.com/WordPress/gutenberg/blob/87fa5f58534281aadd9bdf624f9b8384cfe66c89/packages/edit-post/package.json#L35
During the publishing process, Lerna replaces the local path with the latest version of @wordpress/data that exists. So the question is whether Lerna would replace the path with a version number in the first place. If it would, what would be the version specified and if it makes things any better. In general, it requires some experimentation.

There are other packages that register a data store and provide various services for the app

For the record, there is a convention that a package should declare another package as a NPM dependency even if it merely uses a store registered by it, without doing any import. @youknowriad did a PR that added some missing ones: #23517.

To be clear, all packages that use a given datastore should import it in the entry point by convention. It's something that could be improved because if it's missed it might indeed cause errors. At the moment, there is no simple way to validate it happens. Example:
https://github.com/WordPress/gutenberg/blob/87fa5f58534281aadd9bdf624f9b8384cfe66c89/packages/edit-post/src/index.js#L4-L10

[jsnajdr]

@gziolo If I understand correctly, your concerns are:

• if npm v7 starts installing peerDependencies automatically, then what's the point of changing the declarations from dependencies to peerDependencies in the first place? If a package foo requires @wordpress/data@^1.2.3 as a peer, and an older @wordpress/[email protected] is already in the root, then npm 7 will install a duplicate copy inside foo/node_modules instead of just warning about it. No improvement at all.
• for a peer dependency, will Lerna replace "file:../data" with the version declared in ../data/package.json when publishing? Just like it does for regular dependencies?

These are both good questions and I don't know the answers at this moment. Totally worth testing. It might invalidate the entire peerDependencies idea.

[gziolo]

@jsnajdr, a very good summary of what I shared. I don't have answers as well, we need to investigate all that.

[gziolo]

There are other packages that register a data store and provide various services for the app

For the record, there is a convention that a package should declare another package as a NPM dependency even if it merely uses a store registered by it, without doing any import. @youknowriad did a PR that added some missing ones: #23517.

To be clear, all packages that use a given datastore should import it in the entry point by convention. It's something that could be improved because if it's missed it might indeed cause errors. At the moment, there is no simple way to validate it happens. Example:
https://github.com/WordPress/gutenberg/blob/87fa5f58534281aadd9bdf624f9b8384cfe66c89/packages/edit-post/src/index.js#L4-L10

I started a quick PR that could help to mitigate the issue with import statements for the packages that expose stores used in the package. In the long run, it isn't solid enough, which I was able to confirm by only testing one package @wordpress/viewport in #26655. I propose that we expose the store object from every package and use it as a param that gets passed to select and dispatch calls rather than hardcoded strings. To make it backward compatible we can inject toString method that returns the name of the registered store and cast the store object when passing to select and dispatch calls.

[jsnajdr]

@gziolo I finally have some answers to the npm7 and Lerna questions 🙂

npm 7 installing peer dependencies by default
I did an experiment with eslint-plugin-react which has a peer dependency on eslint, and supports major versions from 3 through 7.

If my project has only eslint-plugin-react@^7 in dependencies and eslint is not there, then npm 6 will not install eslint and will issue a warning:

`[email protected]` requires a peer of eslint@^3 || ^4 || ^5 || ^6 || ^7 but none is installed. You must install peer dependencies yourself.

Doing the same install with npm 7 will automatically install [email protected] into node_modules. Silently, without any warning. That's expected behavior of the new feature. I can run eslint now without worry.

It gets more interesting when my package.json declares two incompatible dependencies:

"dependencies": {
  "eslint": "^2",
  "eslint-plugin-react": "^7"
}

Here npm 6 installs both eslint@2 and eslint-plugin-react@7, although they're incompatible. And issues the same warning about unmatched peer deps as above. Running eslint is very likely to fail at runtime.

npm 7 is much more strict with this scenario. It detects the incompatibility, refuses to install an extra copy of eslint@7 in node_modules/eslint-react-plugin/node_modules, and aborts the entire install process:

npm ERR! code ERESOLVE
npm ERR! ERESOLVE unable to resolve dependency tree
npm ERR! 
npm ERR! While resolving: app@undefined
npm ERR! Found: [email protected]
npm ERR! node_modules/eslint
npm ERR!   eslint@"^2.0.0" from the root project
npm ERR! 
npm ERR! Could not resolve dependency:
npm ERR! peer eslint@"^3 || ^4 || ^5 || ^6 || ^7" from [email protected]
npm ERR! node_modules/eslint-plugin-react
npm ERR!   eslint-plugin-react@"^7.21.5" from the root project
npm ERR! 
npm ERR! Fix the upstream dependency conflict, or retry
npm ERR! this command with --force, or --legacy-peer-deps
npm ERR! to accept an incorrect (and potentially broken) dependency resolution.

Therefore, declaring eslint as peer dependency, as opposed to a normal dependency, has a strong and useful effect, even more so with npm 7. The package manager won't install a duplicate version of the incompatible package, and npm 7 will refuse to install the node_module tree at all if there's a conflict detected.

If @wordpress packages declare plugin-like dependencies, e.g., @wordpress/hooks, as peers, it will make consumers' life easier with both npm 6 and 7, and even more so with v7.

Lerna replacing file: links with real versions when publishing
This is more complicated and the news are not so good. When publishing, Lerna will not touch peerDependencies at all. The version updating on publish has been removed in https://github.com/lerna/lerna/pull/1187, and the rationale was discussed in https://github.com/lerna/lerna/issues/1018.

In short, peer dependencies version range should be as loose as possible, and removing previously compatible versions from the bottom of the range is a major semver change. Because the peer range is part of the module's public API. And the upgrade is no longer a drop-in replacement, but requires upgrades of other packages.

Therefore, we should declare peer dependencies on, e.g., @wordpress/hooks, not by specifying the latest version, but by manually specifying the oldest version that is compatible.

[youknowriad]

Thanks for diving in @jsnajdr declaring peer dependencies definitely seems like a step in the right direction in that case.
Managing dependencies versions become more "manual" but I wonder if that's a good thing in the end. It does add another thing to think about when adding changes (especially the backward-incompatible ones) but it seems the only way to figure it out is to try it?

[gziolo]

@jsnajdr, it all sounds very promising. Great work exploring all options. It makes me wonder if we could automate ourselves the process for keeping peerDependencies up to date.

One challenge I can think of is how we ensure that we always bump a minimum required version when we use the newly introduced API method which isn't a breaking change for the peer package, but would be for the package that is consuming it if it isn't using the version that contains it. To give an example:

• @wordpress/editor depends on a peer dependency @wordpress/hooks at version >= 2 when it's at v2.2.1
• @wordpres/hooks gets new minor version v2.3.0 with a new API method callMe
• @wordpress/editor uses callMe
• now we should enforce the peer dependency at >= 2.3.0

Is it a correct assumption?

[jsnajdr]

Is it a correct assumption?

Yes! Additionally, @wordpress/editor should bump its major version when it starts using the new @wordpress/hooks API. The rationale is that upgrading @wordpress/editor is no longer an innocent and backward-compatible upgrade, as a minor or patch version bump would guarantee. If your app was using an old, no-longer-compatible version of @wordpress/hooks, you'll need to do extra work to finish the @wordpress/editor upgrade. Just like when doing a major version upgrade.

We have a recent practical example of this: data stores created by @wordpress/data now support some built-it controls, select, dispatch and resolveSelect, and consumer packages were upgraded to use the new controls. That means that @wordpress/data should get a minor version bump (new API, backward compatible) and all consumer packages should get a major version bump. I didn't know this at the time when I was making the changes.