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

Framework: Introduce the data module #3832

Merged
merged 6 commits into from Dec 18, 2017

Conversation

@youknowriad
Contributor

youknowriad commented Dec 6, 2017

refs #3805

Still in progress but Feedback appreciated.

This PR introduces a data module centralizing the state of separate redux modules (and later allowing access to state for plugins)

For now, the API mimics the redux API:

wp.data = { subscribe, getState, dispatch, registerReducer };

It also uses Redux internally.

Challenges:

  • What about effects, do we assume all modules use refx? Should we register the effects as well?

My proposed solution is that the data module should only care about the data right now, not the side effects. (This decision is subject to discussion and change later). To still allow using the refx middleware for the editor module, I was able to enhance the "dispatch" function.

  • What about getState? For now it's up to the module author, you can access the whole state and I've used a technique in the editor module to only return the state from the current module when using connect. This avoids refactoring all the components/selectors.

  • What about the custom middlewares like multi. Should they be "global". I assumed the data module should be as small as possible and each module can apply these custom middlewares to its own dispatchcalls if needed.

  • What about the store enhancers: persist, redux-responsive. We can't use store enhancers in an already instantiated store. So if these enhancers are not "global" (like I assumed), you have to find alternatives. I came up with small alternatives for those in the current editor module. (cc @jorgefilipecosta not sure I've everything set up properly for the mobile enhancer, help welcome here)

  • Store persist can be offered as an "option" when registering the reducer, It's simple enought to do, just left this decision for later, maybe it's not needed after all.

  • This PR doesn't address the "expose state to plugins and other modules" issue but it get us closer to a solution to this problem. We need to answer the question of "how to expose this state?" I think the use of the global getState should be discouraged to allow changing the shape of the state in the future without BC. The options here are:

    • Expose selectors (or selectors like without the first "state" parameter) (which means we need to register selectors)
    • Expose a GraphQL like API, I'm thinking we should explore this option even if it seems a hype technology because its external API is really great and we do not have to expose hundreds of selectors. (We'll have to register a resolver to achieve this).

Anyway exposing the state should be resolved as a follow-up PR.

Todo

  • Fix Tests and add new tests
  • Add documentation

@youknowriad youknowriad self-assigned this Dec 6, 2017

@youknowriad youknowriad requested review from georgeh, mcsf, mtias, gziolo, aduth and iseulde Dec 6, 2017

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad
Contributor

youknowriad commented Dec 6, 2017

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Dec 6, 2017

Contributor

@gziolo I don't think it's a good idea, because it could impact other modules. I think we should use a middleware globally only if it's something useful for all modules. Also, I'm hesitant to introduce such a concept to the data module because it's too redux specific and we may want to keep the
data module as "pure" as possible.

Contributor

youknowriad commented Dec 6, 2017

@gziolo I don't think it's a good idea, because it could impact other modules. I think we should use a middleware globally only if it's something useful for all modules. Also, I'm hesitant to introduce such a concept to the data module because it's too redux specific and we may want to keep the
data module as "pure" as possible.

@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Dec 6, 2017

Member

I did some tests and it looks like this works well. Nice work here 👍
I only noticed a small regression when we open the sidebar on mobile and then we reload the sidebar continues open. So our mobile middleware is not taking effect. It's a small problem and I would be ok with focusing/advancing in this approach and deal with this problem right after.

Member

jorgefilipecosta commented Dec 6, 2017

I did some tests and it looks like this works well. Nice work here 👍
I only noticed a small regression when we open the sidebar on mobile and then we reload the sidebar continues open. So our mobile middleware is not taking effect. It's a small problem and I would be ok with focusing/advancing in this approach and deal with this problem right after.

@atimmer

atimmer requested changes Dec 7, 2017 edited

  • For global/local middlewares: I think it should be dependant on the middleware. For example, a redux logger should be global, because you want to log everything. I assume the current middlewares are pretty specific for the editor, so no global middlewares, for now, makes sense.
  • It is really nice that persist can be an option. That is a very clean API and makes persisting to local storage a breeze for plugin authors.
  • I am in favor of exploring a GraphQL API. However, we have to see it to compare both options and make a good decision.
  • I really like that you are dispatching an action when a reducer is registered.

This is a good step towards getting a great API to get to the state. This also means that in theory plugins could to wp.data.getState() already, is this an issue or do you think because Gutenberg is in beta we can break this API anytime we want?

We could consider making this 'private' and only expose the function with wp.data._getState() which should reflect to people that you should not use it yet.

Show outdated Hide outdated data/index.js Outdated
Show outdated Hide outdated data/index.js Outdated
Show outdated Hide outdated data/index.js Outdated
Show outdated Hide outdated editor/store/browser.js Outdated
Show outdated Hide outdated editor/store/persist.js Outdated
Show outdated Hide outdated editor/store/reducer.js Outdated
Show outdated Hide outdated editor/store/store.js Outdated
@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Dec 7, 2017

Contributor

Thanks for the review @atimmer

This also means that in theory plugins could to wp.data.getState() already, is this an issue or do you think because Gutenberg is in beta we can break this API anytime we want?

We could consider making this 'private' and only expose the function with wp.data._getState() which should reflect to people that you should not use it yet.

Yep, I think this should be a private API only accessible to the module defining this state, but not sure how to make this clear. I wonder if there's a way to enforce it by making it more difficult to use :)

Contributor

youknowriad commented Dec 7, 2017

Thanks for the review @atimmer

This also means that in theory plugins could to wp.data.getState() already, is this an issue or do you think because Gutenberg is in beta we can break this API anytime we want?

We could consider making this 'private' and only expose the function with wp.data._getState() which should reflect to people that you should not use it yet.

Yep, I think this should be a private API only accessible to the module defining this state, but not sure how to make this clear. I wonder if there's a way to enforce it by making it more difficult to use :)

Show outdated Hide outdated data/index.js Outdated
Show outdated Hide outdated data/index.js Outdated
Show outdated Hide outdated data/index.js Outdated
multi,
];
const enhancedGetState = () => get( getState(), 'core/editor' );

This comment has been minimized.

@aduth

aduth Dec 11, 2017

Member

Instead of restricting by overriding getState, do you think in this model of data management we might want to create our own variant of connect which accepts the sub-tree to select from?

connect( 'core/editor' )( mapStateToProps )( MyComponent )
@aduth

aduth Dec 11, 2017

Member

Instead of restricting by overriding getState, do you think in this model of data management we might want to create our own variant of connect which accepts the sub-tree to select from?

connect( 'core/editor' )( mapStateToProps )( MyComponent )

This comment has been minimized.

@youknowriad

youknowriad Dec 12, 2017

Contributor

Yeah, no real preference here on my side, I do think your option is more flexible especially when we'll start to query other modules' state. This was just a convenience for now to avoid changing all the components.

@youknowriad

youknowriad Dec 12, 2017

Contributor

Yeah, no real preference here on my side, I do think your option is more flexible especially when we'll start to query other modules' state. This was just a convenience for now to avoid changing all the components.

@@ -20,7 +20,7 @@ import {
* Internal Dependencies
*/
import { setupEditor, undo } from '../../actions';
import createReduxStore from '../../store';
import store from '../../store';

This comment has been minimized.

@aduth

aduth Dec 11, 2017

Member

It's a bit unnerving to me that we treat store as a singleton instance, since there's nothing stopping someone from importing from store to get state or dispatch from anywhere in the codebase. This makes it a little less predictable how state is being interacted with. In this way, it's more similar to the old Flux dispatcher approach. One of the reasons we moved away from Flux to Redux in Calypso was that sharing the store as a singleton could lead to session data leaking between server-side requests. Obviously this doesn't apply to the purely client-side editor, so it may not be as big a concern.

@aduth

aduth Dec 11, 2017

Member

It's a bit unnerving to me that we treat store as a singleton instance, since there's nothing stopping someone from importing from store to get state or dispatch from anywhere in the codebase. This makes it a little less predictable how state is being interacted with. In this way, it's more similar to the old Flux dispatcher approach. One of the reasons we moved away from Flux to Redux in Calypso was that sharing the store as a singleton could lead to session data leaking between server-side requests. Obviously this doesn't apply to the purely client-side editor, so it may not be as big a concern.

This comment has been minimized.

@youknowriad

youknowriad Dec 12, 2017

Contributor

The main idea here is that a WP module requiring state should register itself as soon as the script is loaded (other plugins might depend on this module, thus, on its state).

A module = 1 state tree (or 0).

We shouldn't wait for the provider to be mounted to load initialize the state of the module.

So this PR is changing the meaning of the "editor" state from editor instance's state to editor module's state which I think is the way to go to "modularize" the state in a centralized "data" module.

Later, we can decide to add a root level to the "editor" module's reducer containting the "id" of the instanciated editor (which would mean we could hold multiple editor instances)

@youknowriad

youknowriad Dec 12, 2017

Contributor

The main idea here is that a WP module requiring state should register itself as soon as the script is loaded (other plugins might depend on this module, thus, on its state).

A module = 1 state tree (or 0).

We shouldn't wait for the provider to be mounted to load initialize the state of the module.

So this PR is changing the meaning of the "editor" state from editor instance's state to editor module's state which I think is the way to go to "modularize" the state in a centralized "data" module.

Later, we can decide to add a root level to the "editor" module's reducer containting the "id" of the instanciated editor (which would mean we could hold multiple editor instances)

* @param {String} key Reducer key
* @param {Object} reducer Reducer function
*/
export function registerReducer( key, reducer ) {

This comment has been minimized.

@aduth

aduth Dec 11, 2017

Member

To address the challenges around module-specific middlewares and enhancers, could the data module serve as a registry of stores, rather than of reducers? So each module could have its own middlewares, etc. Main difference would be subscribe, dispatch, and getState would need to operate separately on each store, rather than assuming there's a single store object to work with.

@aduth

aduth Dec 11, 2017

Member

To address the challenges around module-specific middlewares and enhancers, could the data module serve as a registry of stores, rather than of reducers? So each module could have its own middlewares, etc. Main difference would be subscribe, dispatch, and getState would need to operate separately on each store, rather than assuming there's a single store object to work with.

This comment has been minimized.

@youknowriad

youknowriad Dec 12, 2017

Contributor

Do you think we'd have use-cases requiring cross-module selectors. For example a post-editor module declares a reducer and is dependent on the editor module. In which case, post-editor could provide a cross-module selector? It also seem helpful for plugins.

Granted, you can recreate this with three selectors, one for each store and a third function to merge both results.


Also, the other question I have is whether the data module should assume a redux store (with middlewares, enhancers) where a reducer is not really redux-related. Thinking the data module should not be framework-dependent in its external API.

@youknowriad

youknowriad Dec 12, 2017

Contributor

Do you think we'd have use-cases requiring cross-module selectors. For example a post-editor module declares a reducer and is dependent on the editor module. In which case, post-editor could provide a cross-module selector? It also seem helpful for plugins.

Granted, you can recreate this with three selectors, one for each store and a third function to merge both results.


Also, the other question I have is whether the data module should assume a redux store (with middlewares, enhancers) where a reducer is not really redux-related. Thinking the data module should not be framework-dependent in its external API.

This comment has been minimized.

@mcsf

mcsf Dec 12, 2017

Contributor

I have a few concerns with the registry-of-stores model, as that would break the premise of a single aggregator and pipeline of state for the entire application. If the goal were to provide a common framework for independent plugins to run on, I think it'd make more sense, but here we expect plugins to build on top of one another, and we expect there to be communication to and from Gutenberg, correct?

That said, I think I understand wanting to separate things a little, but to me that would be achieved by keeping enhancements and middlewares domain-specific.

Also, the other question I have is whether the data module should assume a redux store (with middlewares, enhancers) where a reducer is not really redux-related. Thinking the data module should not be framework-dependent in its external API.

I share this idea, and the broader idea that learning what a reducer is is easier than wrapping one's head around a store.

@mcsf

mcsf Dec 12, 2017

Contributor

I have a few concerns with the registry-of-stores model, as that would break the premise of a single aggregator and pipeline of state for the entire application. If the goal were to provide a common framework for independent plugins to run on, I think it'd make more sense, but here we expect plugins to build on top of one another, and we expect there to be communication to and from Gutenberg, correct?

That said, I think I understand wanting to separate things a little, but to me that would be achieved by keeping enhancements and middlewares domain-specific.

Also, the other question I have is whether the data module should assume a redux store (with middlewares, enhancers) where a reducer is not really redux-related. Thinking the data module should not be framework-dependent in its external API.

I share this idea, and the broader idea that learning what a reducer is is easier than wrapping one's head around a store.

This comment has been minimized.

@aduth

aduth Dec 12, 2017

Member

I'm wondering if we could still implement as separate stores, but keep the same top-level interface. i.e.

wp.data.getState = () => stores.reduce( ( result, { store, key } ) => {
    return { ...result, [ key ]: store.getState() };
}, {} );

Registering still looks like you're registering a reducer, but you'd have the option to specific middlewares or enhancers if needed.

Might be a nightmare for performance.

@aduth

aduth Dec 12, 2017

Member

I'm wondering if we could still implement as separate stores, but keep the same top-level interface. i.e.

wp.data.getState = () => stores.reduce( ( result, { store, key } ) => {
    return { ...result, [ key ]: store.getState() };
}, {} );

Registering still looks like you're registering a reducer, but you'd have the option to specific middlewares or enhancers if needed.

Might be a nightmare for performance.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Dec 13, 2017

Contributor

I think this in a state we can merge and iterate on:

  • It's using a single redux instance but nothing stopping us from changing later
  • getState is available but a proper way to expose state will be added in a follow-up PR
  • I didn't include helpers like HoCs yet but yes, these are welcome. These also raise the question about the dependency towards wp-element. I was hoping we could avoid it, but maybe not.
Contributor

youknowriad commented Dec 13, 2017

I think this in a state we can merge and iterate on:

  • It's using a single redux instance but nothing stopping us from changing later
  • getState is available but a proper way to expose state will be added in a follow-up PR
  • I didn't include helpers like HoCs yet but yes, these are welcome. These also raise the question about the dependency towards wp-element. I was hoping we could avoid it, but maybe not.
Show outdated Hide outdated data/index.js Outdated
@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Dec 13, 2017

Member

I need to think a bit more about this PR. It’s a great start. I would be happy to see it merged very soon and mark as experimental. It worked quite well with other extensibility changes. The sooner we get some community trying to integrate it the better shape it can take. My only concern at the moment is that it’s difficult to predict what happens when a plugin creates their own customized store interface that uses different middlewares. I need to figure out if that can cause some troubles when you dispatch an action that would be enhanced in editor. Let’s say preferences get dispatched from plugin and they wouldn’t trigger enhancers from editor. Need to think a bit more about it.

Member

gziolo commented Dec 13, 2017

I need to think a bit more about this PR. It’s a great start. I would be happy to see it merged very soon and mark as experimental. It worked quite well with other extensibility changes. The sooner we get some community trying to integrate it the better shape it can take. My only concern at the moment is that it’s difficult to predict what happens when a plugin creates their own customized store interface that uses different middlewares. I need to figure out if that can cause some troubles when you dispatch an action that would be enhanced in editor. Let’s say preferences get dispatched from plugin and they wouldn’t trigger enhancers from editor. Need to think a bit more about it.

@aduth

This looks reasonable to me but, yes, we definitely want to seek answers to patterns around middlewares, enhancers, and side effects.

Show outdated Hide outdated data/index.js Outdated
@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Dec 18, 2017

Contributor

@gziolo relying on replaceReducer simplifies the implementation a lot
170ca68 :)

Contributor

youknowriad commented Dec 18, 2017

@gziolo relying on replaceReducer simplifies the implementation a lot
170ca68 :)

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Dec 18, 2017

Member

Glad to see it. Looks quite clean. So the idea would be to register reducer per module/plugin, right? I was wondering how this replaceReducer works in the middle of app's lifecycle. It seems like it magically works according to the official Redux hot-loading example: https://github.com/reactjs/redux/blob/4c646b35c036df590b63fa313659d070f306082b/examples/universal/common/store/configureStore.js.

I will test this PR later today, need to wrap up a few things first.

Member

gziolo commented Dec 18, 2017

Glad to see it. Looks quite clean. So the idea would be to register reducer per module/plugin, right? I was wondering how this replaceReducer works in the middle of app's lifecycle. It seems like it magically works according to the official Redux hot-loading example: https://github.com/reactjs/redux/blob/4c646b35c036df590b63fa313659d070f306082b/examples/universal/common/store/configureStore.js.

I will test this PR later today, need to wrap up a few things first.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Dec 18, 2017

Contributor

Ok! I'm moving forward with this PR. Expect some follow-up PRs/issues on state exposition, effects.

Contributor

youknowriad commented Dec 18, 2017

Ok! I'm moving forward with this PR. Expect some follow-up PRs/issues on state exposition, effects.

@youknowriad youknowriad merged commit 5c33f6f into master Dec 18, 2017

3 checks passed

codecov/project 38.58% (+<.01%) compared to 7c917c9
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@youknowriad youknowriad deleted the add/data-module branch Dec 18, 2017

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Dec 18, 2017

Member

Even better, I will test it together with coediting with simple rebase :D

Member

gziolo commented Dec 18, 2017

Even better, I will test it together with coediting with simple rebase :D

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Dec 18, 2017

Member

Was it deferred to handle moving editor/actions.js, editor/selectors.js, and editor/store-persist.js to the new editor/store directory to a subsequent pull request, or were they intentionally omitted from the move?

Member

aduth commented Dec 18, 2017

Was it deferred to handle moving editor/actions.js, editor/selectors.js, and editor/store-persist.js to the new editor/store directory to a subsequent pull request, or were they intentionally omitted from the move?

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Dec 18, 2017

Contributor

@aduth Yes, mostly deferred because it's too impactful (too many changes) and I didn't want to show it as part of this PR's changes. I'd be happy to do it and merge it as quickly as possible to avoid infinite rebases :)

Contributor

youknowriad commented Dec 18, 2017

@aduth Yes, mostly deferred because it's too impactful (too many changes) and I didn't want to show it as part of this PR's changes. I'd be happy to do it and merge it as quickly as possible to avoid infinite rebases :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment