Data Module: Introduce the "registry" concept #7527

youknowriad · 2018-06-25T10:05:05Z

Related #7453

Right now, the Data module is acting like a global variable holding a list of registered stores. That's super handy for WordPress Core to allow communication and data access between plugins and Core. But this has the downside of a loss in flexibility. For example:

At the moment, the Gutenberg editor manages its state in the core/editor namespace which makes it very hard to allow embedding multiple Gutenberg editors in the same page because they will shared the same store.

In this PR, we're introducing the concept of a registry to solve this:

A registry is a collection of stores
The current wp.data acts as the default registry which means the current behavior of the Data Module won't change.
You can create custom registries and wrap parts of the application in a RegistryProvider, this is very similar to ReduxProvider or any *Provider pattern used in several react libraries.
When you wrap a component (or a tree of components) in a RegistryProvider providing a custom registry. All calls to withSelect/withDispatch within this provider will use the provided registry instead of relying on the global wp.data registry.

Notes

To achieve WIP: Blocks: Reimplement shared block as embedded editor #7453 we need a way to create a registry from another registry and replace one of its stores. I'm intentionally not including such an API in this PR and leaving it for WIP: Blocks: Reimplement shared block as embedded editor #7453 when we'll actually need it.
I planned to update the withSelect/withDispatch unit tests to use the RegistryProvider but since Enzyme doesn't support React context yet, I'm holding off. It's not crucial though because we already test the global registry and createRegistry.

Testing instructions

Just make sure the editor loads like today.

youknowriad · 2018-06-25T10:46:08Z

mmm, it looks like even our current tests are failing because of Enzyme. I wonder how we should move forward with this: Rewrite all these tests using react-test-rendered?

youknowriad · 2018-06-25T12:31:13Z

I updated the tests to use react-test-renderer. It should be fine now.

aduth · 2018-06-25T13:11:02Z

packages/data/src/components/registry-provider.js

@@ -0,0 +1,26 @@
+/**


There's a fair bit of inconsistency in how we're creating components here and with relation to conventions of other modules.

withDispatch and withSelect are in their own files, but withRegistry is combined into the provider file

If we want a pattern of component-per-file, we should be strictly consistent. Case in point: I searched with-registry on this page expecting to find the file where this component was defined, but it did not exist.

Other modules we have folder pattern components/registry-provider/index.js .

We may argue this is the "next step of simplest", but if we're already to the point of splitting by component, we may as well strive for consistency. It also provides an easier pattern for ensuring we're responsible for including README per component.

Other modules we place higher-order components within a nested higher-order folder.

While the naming/placement could be subject to debate, the point is: (a) consistency, (b) accurately reflecting that these aren't actually components in their own right, and can't be used directly, only as an enhancer to another component.

Fair points:

I'm leaning towards:

components/registry-provider/index.js / components/with-select/index.js. I personally don't think the higher-order folder is necessary because sometimes we need both from the same file createContext calls.

I can extract with-registry but my idea was to avoid exporting the Consumer entirely. It's minor though. but even if we separate withRegistry we still need to export two components because createContext produces two components. So not certain how to go with this.

even if we separate withRegistry we still need to export two components because createContext produces two components. So not certain how to go with this.

I think it's fine to have an exception for createContext, mostly out of necessity.

aduth · 2018-06-25T13:13:02Z

packages/data/src/components/registry-provider.js

+	return ( props ) => (
+		<Consumer>
+			{ ( registry ) => (
+				<OriginalComponent registry={ registry } { ...props } />


We'll have the same problem I encountered in #7453, which is that withDispatch and withSelect pass through all props they receive. And for some components this is problematic because they then spread onto the DOM elements (e.g. Button), thus producing warnings about unrecognized attributes.

To preempt the obvious solution to use omit( this.props, [ 'registry' ] ), I prefer my solution in #7453 which is marginally more performant: pass the props unaffected by a separate ownProps prop. This also has the effect of not including registry in mapSelectToProps / mapDispatchToProps.

gziolo · 2018-06-25T13:32:26Z

Just noting, that I very like the fact that we are moving code to their own files. The benefit of this is that we no longer need to mock the function that registers store because it is no longer autoloaded.

aduth · 2018-06-26T14:48:44Z

packages/data/src/components/registry-provider/index.js

+ */
+import { createContext } from '@wordpress/element';
+
+const { Consumer, Provider } = createContext( null );


Rather than having withDispatch and withSelect know about the default registry, could we just assign it as the default here instead of null? Avoids need for special handling, where:

const dispatch = this.props.registry ? this.props.registry.dispatch : defaultRegistry.dispatch;

Becomes:

const { dispatch } = this.props.registry;

I think the issue is that people use withSelect/ withDispatch without using a provider. I'm wondering if the default value is stored in the provider or can be provided by the consumer even without any provider. I'll have to check.

I think the consumer should inherit the default value if there is no ascendant provider.

Demo: https://codepen.io/aduth/pen/rKZrKo

I think the consumer should inherit the default value if there is no ascendant provider.

We use this behavior both in tests and for Disabled component (see #7138). This is why you have to provide this default value when creating context.

If I follow properly, the following should do the trick:

const { Consumer, Provider } = createContext( createRegistry() );

gziolo · 2018-06-27T05:54:29Z

packages/data/src/components/with-dispatch/test/index.js

+			};
+		} )( ( props ) => <button onClick={ props.increment } /> );
+
+		const testRenderer = TestRenderer.create(


My own notes: to replace shallow from enzyme we would need only:

jest.shallow = ( element ) => { const TestRenderer = require( 'react-test-renderer' ); const testRenderer = TestRenderer.create( element ); const testInstance = testRenderer.root; if ( testInstance !== null) { // not sure if it can be null? testInstance.update = testRenderer.update; } return testInstance; };

Not that much of the work needed :)

Actually, it replaces mount as far as I can see. Interesting 🤔

Would be good to explore removing enzyme in lieu of this approach in a separate PR. The lack of React 16 support is frustrating. My only concern is whether or not it makes our tests run significantly slower.

Yes, definitely something we should explore further. Actually, enzyme brings lots of performance penalty on its own. See:
https://github.com/WordPress/packages/blob/master/packages/jest-preset-default/scripts/setup-test-framework.js#L3-L6
We lazy load it only when it is necessary to mitigate that :)
It isn't as much of an issue at the moment, but I could observe a huge difference when dealing with 1 000+ test suites in Calypso.

This should be better now

atimmer · 2018-07-06T13:35:21Z

Looks good. I've tested it with our custom store and it all still works.

I had one question: Does this mean that the data in the core store will now potentially be duplicated between registries, or will we always refer to the wp.data store if we need core data?

aduth · 2018-07-11T16:15:43Z

I took it upon myself to rebase this one, since I'm largely to blame for the conflicts. Wasn't the easiest to rebase, but I think I managed to pull it off in 77f19ca I did it in a separate branch just in case something's wrong with it, otherwise you can rebase and cherry-pick into this one.

One remaining point of feedback I have yet is: In withSelect, with RegistryConsumer passing the registry as a prop, do we need to account for the fact that registry could change? i.e. do we need to unsubscribe and resubscribe to the new registry?

noisysocks · 2018-07-12T02:33:32Z

One remaining point of feedback I have yet is: In withSelect, with RegistryConsumer passing the registry as a prop, do we need to account for the fact that registry could change? i.e. do we need to unsubscribe and resubscribe to the new registry?

I imagine it's very rare that one would dynamically change the registry fed into RegistryProvider. If it's easy to support, then let's do it. If it's not, then maybe a console.warn would suffice?

noisysocks

Love this refactor! I'm not seeing any regressions when I test locally 👍

Left a few comments—nothing blocking.

noisysocks · 2018-07-12T01:55:15Z

packages/data/src/index.js

-}
+export { default as withSelect } from './components/with-select';
+export { default as withDispatch } from './components/with-dispatch';
+export { default as RegistryProvider } from './components/registry-provider';


Should we export createRegistry from ./registry so that third party developers can create their own registries?

noisysocks · 2018-07-12T01:59:32Z

packages/data/src/registry.js

+	}
+
+	Object.entries( {
+		'core/data': dataStore,


Is it right that every registry created has a core/data store? Or should we instead make only the default registry have this? e.g.

// ./registry.js: Object.entries( storeConfig ).map( ( [ name, config ] ) => registerStore( name, config ) ); // ./default-registry.js: import dataStore from './store'; export default createRegistry( { 'core/data': dataStore } );

Every registry should have a dataStore IMO to keep track of resolvers resolution.

noisysocks · 2018-07-12T02:16:20Z

packages/data/src/components/with-dispatch/test/index.js

+describe( 'withDispatch', () => {
+	let registry;
+	beforeEach( () => {
+		registry = createRegistry();


I love how this approach makes these unit tests not rely on any global state 😍

noisysocks · 2018-07-12T02:30:02Z

packages/data/src/components/with-select/test/index.js

+		// wp.data export. But in-fact, this serves as a good deterrent for
+		// including both `withSelect` and `select` in the same scope, which
+		// shouldn't occur for a typical component, and if it did might wrongly
+		// encourage the developer to use `select` within the component itself.


Bit confused by this. Are we saying that all users of withSelect should name the first callback argument _select? Why say this in a unit test and not in the README?

We already have a lint rule in Gutenberg which would prevent the select in import { select } from '@wordpress/data' from accidentally being shadowed by withSelect.

I think we're saying that we should prefer withSelect over select in most cases.

But why name the callback argument _select?

The idea is that we discourage select in components by virtue of the shadowing lint rule.

This code is bad and will be made obviously bad to the developer by lint warning between select the import and select the argument:

import { select, withSelect } from '@wordpress/data'; class Foo extends Component { myFunction() { // ... select( 'core/editor' ).getSomething(); } } export default withSelect( ( select ) => { // ... } )( Foo );

In the tests, we don't care (because we're testing both select and withSelect), which is why we're okay to do the ugly aliasing. The comment is meant to explain this, but apparently does a poor job of doing so.

That makes sense! It hadn't occurred to me that we're testing both select and withSelect in these tests 🤦‍♂️

youknowriad · 2018-07-12T12:10:56Z

I might need some help rebasing this @aduth I don't want to mess up with the latest changes :)

Edit: Sorry, I missed the comment above, it should be fine.

aduth · 2018-07-12T12:58:26Z

One remaining point of feedback I have yet is: In withSelect, with RegistryConsumer passing the registry as a prop, do we need to account for the fact that registry could change? i.e. do we need to unsubscribe and resubscribe to the new registry?

I imagine it's very rare that one would dynamically change the registry fed into RegistryProvider. If it's easy to support, then let's do it. If it's not, then maybe a console.warn would suffice?

I wonder if we could use some key trickery to force the unmount when the registry changes, like what we do in RichText for TinyMCE:

gutenberg/editor/components/rich-text/index.js

Lines 859 to 862 in 81983b7

    
           // Generating a key that includes `tagName` ensures that if the tag 
        
           // changes, we unmount and destroy the previous TinyMCE element, then 
        
           // mount and initialize a new child element in its place. 
        
           const key = [ 'editor', Tagname ].join();

youknowriad · 2018-07-12T14:55:27Z

I updated with the remountOnPropChange HoC which remounts the withSelect/withDispatch when the registry prop changes. It should be ready to go

aduth · 2018-07-12T15:10:53Z

packages/data/src/components/remountOnPropChange/index.js

@@ -0,0 +1,42 @@
+/**


This folder name should be kebab-case, remount-on-prop-change

yes, I just noticed it too. This will be fixed in the @wordpress/compose PR I'm working on

gziolo · 2018-07-12T17:30:59Z

Should we document new behavior and method?

youknowriad added the [Package] Data /packages/data label Jun 25, 2018

youknowriad self-assigned this Jun 25, 2018

youknowriad requested review from aduth and a team June 25, 2018 10:05

aduth reviewed Jun 25, 2018

View reviewed changes

aduth previously requested changes Jun 26, 2018

View reviewed changes

gziolo reviewed Jun 27, 2018

View reviewed changes

youknowriad force-pushed the try/data-registry branch from 39f4a9f to 0e46b9a Compare June 27, 2018 12:23

atimmer approved these changes Jul 6, 2018

View reviewed changes

noisysocks approved these changes Jul 12, 2018

View reviewed changes

youknowriad force-pushed the try/data-registry branch 2 times, most recently from 57cb9ce to 775119b Compare July 12, 2018 12:58

aduth and others added 2 commits July 12, 2018 14:49

Data: Move registry into own file

7db5542

Export the "createRegistry" function

80376c6

youknowriad force-pushed the try/data-registry branch from 775119b to 80376c6 Compare July 12, 2018 13:49

Adding remountOnPropChange HoC

e6eaab9

youknowriad merged commit 5fb24e4 into master Jul 12, 2018

aduth reviewed Jul 12, 2018

View reviewed changes

This was referenced Jul 12, 2018

WIP: Blocks: Reimplement shared block as embedded editor #7453

Closed

Editor: Move media upload from utils #7978

Merged

mtias added this to the 3.3 milestone Jul 20, 2018

youknowriad deleted the try/data-registry branch January 23, 2019 07:58

Data Module: Introduce the "registry" concept #7527

Data Module: Introduce the "registry" concept #7527

Conversation

youknowriad commented Jun 25, 2018

youknowriad commented Jun 25, 2018

youknowriad commented Jun 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented Jun 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo Jun 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo Jun 27, 2018 • edited Loading

Choose a reason for hiding this comment

atimmer commented Jul 6, 2018

aduth commented Jul 11, 2018 • edited Loading

noisysocks commented Jul 12, 2018

noisysocks left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noisysocks Jul 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented Jul 12, 2018 • edited Loading

aduth commented Jul 12, 2018 • edited Loading

youknowriad commented Jul 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented Jul 12, 2018

gziolo Jun 27, 2018 •

edited

Loading

gziolo Jun 27, 2018 •

edited

Loading

aduth commented Jul 11, 2018 •

edited

Loading

noisysocks left a comment •

edited

Loading

noisysocks Jul 12, 2018 •

edited

Loading

youknowriad commented Jul 12, 2018 •

edited

Loading

aduth commented Jul 12, 2018 •

edited

Loading