Data Module: Restrict the state access to the module registering the reducer only

[youknowriad]

This PR updates the API of the "data" module to restrict the access to the global state.
The module registering a reducer receives a "store" object as a return value to the registerReducer call.

Coming next

• An official way to expose a module's data to other modules.

Testing instructions

• Test that the editor loads and saves posts properly.

[youknowriad]

cc @atimmer

[aduth]

An official way to expose a module's data to other modules.

Do you have any ideas to share about how this looks? I would have thought one of the ideas with a global store is that the state is easily accessible from anywhere in the system.

[aduth]

Is get necessary here (or anywhere we're calling getState())?

[gziolo]

You can introduce the following helper method:

const getState = () => get( store.getState(), key );

and use in all places inside registerReducer.

[youknowriad]

I guess it's not necessary after all

[gziolo]

Do you have any ideas to share about how this looks? I would have thought one of the ideas with a global store is that the state is easily accessible from anywhere in the system.

Yes, I would love to see that explained. I wanted to integrate it for coediting branch today, but I had no clue how to register local state and access it from Redux afterward. I would love to see some example with the code to better understand the idea.

[gziolo]

wp.data.registerReducer - should it have a name passed as a first param? I guess it should. I noticed it today, when reading docs.

[atimmer]

I think this PR can go in. If we desire to have the store globally available later, we can always revert this. Adding more APIs is easier than restricting. I'm assuming

An official way to expose a module's data to other modules.

will come very soon after this PR has been merged.

[youknowriad]

Do you have any ideas to share about how this looks?

I have rough ideas and some POCs. And it's two separate ideas:

• register a subtree of resolvers and provide a GraphQL API (Provider and query HoC)
• register selectors and provide a (Provider and withSelectors HoC)

I may submit a POC tomorrow.

[gziolo]

Do we have 3 enhancers in here?:

• applyMiddlewares
• loadAndPersist
• enhanceWithBrowserSize

Can we further refactor this code to make it more obvious that we modify store instance?

[youknowriad]

The store instance is only modified in applyMiddlewares

[gziolo]

Right, what about having:

const store = applyMiddlewares(
    registerReducer( 'core/editor', withRehydratation( reducer, 'preferences' ) )
);
enhanceWithLoadAndPersist( store, 'preferences', STORAGE_KEY, PREFERENCES_DEFAULTS );
enhanceWithBrowserSize( store );

[gziolo]

I'm trying to understand why using store.subscribe is not enough here. When I look at Redux source code, I see that listener is executed only at the end of the dispatch call: https://github.com/reactjs/redux/blob/master/src/createStore.js#L194. This code tries to filter out all listener calls when the local state hasn't changed. It seems like a good idea, but the question is how it works in practice? Is Redux really immutable here? It might be not architected this way to speed up building new state.

[youknowriad]

The more we'd have module and plugins using the "data" module, the more we'd have actions handled only by a separate reducer which would create unnecessary listener calls for most modules/plugins. I think this is a necessary performance improvement.

Not noticeable right now because we only have one registered reducer.

[gziolo]

Okey, I looked into combineReducers code. It seems to be smart enough to return old state when nothing has changed. See: https://github.com/reactjs/redux/blob/master/src/combineReducers.js#L118.

[youknowriad]

Yes, but in this case the state did change but only for one module.

[gziolo]

This change makes a lot of sense. I left some comments when trying to understand how it is supposed to work. Feel free to ignore my code refactorings proposed, if you don't like them. They helped me to understand better what is happening, but it might be an individual thing.

The only thing that needs to be updated before merging is this issues I raised in the comment for docs: https://github.com/WordPress/gutenberg/pull/4058/files#r157513975.