Data Module: Restrict the state access to the module registering the reducer only by youknowriad · Pull Request #4058 · WordPress/gutenberg

youknowriad · 2017-12-18T09:07:24Z

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 · 2017-12-18T09:07:32Z

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 · 2017-12-18T15:18:36Z

data/index.js

-export const subscribe = store.subscribe;
+	return {
+		dispatch: store.dispatch,
+		getState: () => get( store.getState(), key ),


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

You can introduce the following helper method:

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

and use in all places inside registerReducer.

I guess it's not necessary after all

gziolo · 2017-12-18T15:23:13Z

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 · 2017-12-18T15:24:49Z

data/README.md

 If your module or plugin needs to store and manipulate client-side data, you'll have to register a "reducer" to do so. A reducer is a function taking the previous `state` and `action` and returns an update `state`. You can learn more about reducers on the [Redux Documentation](https://redux.js.org/docs/basics/Reducers.html)

-### `wp.data.getState()`
+This function returns a Redux-like store object with the following methods:


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 · 2017-12-18T15:39:23Z

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 · 2017-12-18T15:46:04Z

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 · 2017-12-20T09:17:50Z

editor/store/index.js


-registerReducer( 'core/editor', withRehydratation( reducer, 'preferences' ) );
+let store = registerReducer( 'core/editor', withRehydratation( reducer, 'preferences' ) );
+store = applyMiddlewares( store );


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?

The store instance is only modified in applyMiddlewares

Right, what about having:

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

gziolo · 2017-12-20T09:46:21Z

data/index.js

+	return {
+		dispatch: store.dispatch,
+		getState: () => get( store.getState(), key ),
+		subscribe( listener ) {


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.

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.

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.

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.

…reducer only

youknowriad added the Framework Issues related to broader framework topics, especially as it relates to javascript label Dec 18, 2017

youknowriad self-assigned this Dec 18, 2017

youknowriad requested review from aduth, gziolo and mcsf December 18, 2017 09:07

aduth reviewed Dec 18, 2017

View reviewed changes

gziolo reviewed Dec 18, 2017

View reviewed changes

gziolo reviewed Dec 20, 2017

View reviewed changes

gziolo approved these changes Dec 20, 2017

View reviewed changes

Data Module: Restrict the state access to the module registering the …

ee520c3

…reducer only

youknowriad force-pushed the update/restrict-global-state-access branch from 7b6964a to ee520c3 Compare December 20, 2017 10:32

youknowriad merged commit 34a13d3 into master Dec 20, 2017

youknowriad deleted the update/restrict-global-state-access branch December 20, 2017 10:40

gziolo mentioned this pull request Dec 20, 2017

Data Module: Expose state using selectors #4105

Merged

Conversation

youknowriad commented Dec 18, 2017

Uh oh!

youknowriad commented Dec 18, 2017

Uh oh!

aduth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gziolo Dec 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gziolo commented Dec 18, 2017

Uh oh!

gziolo Dec 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atimmer commented Dec 18, 2017

Uh oh!

youknowriad commented Dec 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowriad Dec 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gziolo left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gziolo Dec 20, 2017 •

edited

Loading

gziolo Dec 18, 2017 •

edited

Loading

youknowriad Dec 20, 2017 •

edited

Loading