SDK: Try wp.data with calypso state tree #26838

coderkevin · 2018-08-23T04:05:36Z

This creates a custom-store-plugin which is a general-purpose plugin which allows any pre-existing redux store to be used for a given wp.data store instead of wp.data creating one itself. This allows for two things:

Stores can use enhancers and middleware, which is not possible in the
default wp.data configuration.
wp.data stores can share redux stores.

Note: This also creates an internals-plugin, which is necessary to avoid the reimplementing of internals within the calypso-registry-plugin. The use function of the internals-plugin
will be submitted as a PR to core Gutenberg as well so perhaps it will not be necessary for long.

For review purposes, the internals-plugin.js implementation is identical to the @wordpress/data registry implementation except for the use function.

To Test:

Start calypso in local dev mode as usual.
Go to http://calypso.localhost:3000/me/account and change the "Admin Color Scheme"
Behavior should be the same, but it's using withSelect now.

Note: CI Failure is because of a reference to window in the @wordpress/data registry code. I've added a PR to fix this: WordPress/gutenberg#9261

matticbot · 2018-08-23T04:05:37Z

Test live: https://calypso.live/?branch=try/wp-data-calypso-plugin

ockham · 2018-08-23T08:33:18Z

Note: CI Failure is because of a reference to window in the @wordpress/data registry code. I've added a PR to fix this: WordPress/gutenberg#9261

Noting here that I can't boot Calypso on this branch because of that.

ockham · 2018-08-23T08:38:15Z

Applied https://github.com/WordPress/gutenberg/pull/9261/files#r212224119 locally.

Now failing to boot because of https://github.com/WordPress/gutenberg/blob/a6c36e53974ac8a9fdd6163e61b54064cfd8910f/packages/compose/src/with-safe-timeout/index.js#L19

Edit: Filed WordPress/gutenberg#9265

jsnajdr · 2018-08-23T09:01:06Z

client/state/wp-data/calypso-wp-data-provider.js

+	}
+
+	componentDidMount() {
+		this.updateRegistry( this.props.calypsoStore );


It's better to init the registry in the component constructor. Then it will get initialized in server side rendering, too. componentDid{Mount,Update} is never called in SSR and the registry will remain undefined.

If we do that, then I'll need to make sure the store prop never changes. Is that what we want to do?

Also, the calypsoStore prop doesn't exist when the constructor is run, so it would need to be stitched up afterwards.

If we do that, then I'll need to make sure the store prop never changes. Is that what we want to do?

The componentDidUpdate method will still be there to take care of the prop change. I'm suggesting only merging componentDidMount into the constructor. The constructor is the only one that gets called in both SSR and DOM rendering.

Also, the calypsoStore prop doesn't exist when the constructor is run, so it would need to be stitched up afterwards.

That's strange. Are you sure it doesn't exist? Getting this.props initialized is the (only) reason why React component constructors often call super( props ):

constructor( props ) { // this.props is undefined on this line super( props ); // the super call just initialized them! this.registry = this.updateRegistry( this.props.calypsoStore ); }

Is you constructor doing the super call?

I just checked again, and calypsoStore is in fact coming through on the constructor. I'll adjust this to remove componentDidMount then. Thanks!

ockham · 2018-08-23T10:54:17Z

client/state/wp-data/calypso-registry-plugin.js

+	calypsoStore.subscribe( () => {
+		const calypsoState = calypsoStore.getState();
+		const hasChanged = calypsoState !== lastCalypsoState;
+		lastCalypsoState = calypsoState;


This assigment could also go into the if clause below, no?

Yes, that would be better, thanks!

ockham · 2018-08-23T11:46:40Z

client/state/wp-data/calypso-registry-plugin.js

+		listeners.push( listener );
+		const registryUnsub = registry.subscribe( listener );
+
+		return () => {


Maybe add a comment above that we're returning the unsubscribe function here? Or maybe just JSDoc the public methods?

Addressed in 20b3b95

ockham · 2018-08-23T11:52:08Z

client/state/wp-data/calypso-wp-data-store.js

+import { getPreference, isFetchingPreferences } from 'state/preferences/selectors';
+
+export default {
+	useCalypsoStore: true,


Can we drop this bool flag, and just wrap the object in use, rather than having the consumer do that? I think that would make a bit clearer what we're doing here.

I'm not sure what you mean by this. Can you elaborate or share a code snippet?

ockham · 2018-08-23T13:45:11Z

client/state/wp-data/calypso-registry-plugin.js

+
+	function subscribe( listener ) {
+		listeners.push( listener );
+		const registryUnsub = registry.subscribe( listener );


Now that I think about it -- do we even have to take care of passing listeners on to the underlying registry? It'd be great if we only needed to handle Calypso specific ones...

If we ever want non-calypso stores to be registered, we'll have to do this.

ockham · 2018-08-23T13:52:54Z

This seems to be working quite well!

For completeness' sake, do we want to add a resolver to this PoC, and remove the query component?

tyxla · 2018-08-23T14:11:32Z

client/state/wp-data/calypso-wp-data-store.js

+/**
+ * Internal dependencies
+ */
+import { fetchPreferences, setPreference, savePreference } from 'state/preferences/actions';


Would be great if we can have our own resolvers, and not rely on any wpcom.undocumented requests.

Looks like we could use apiFetch() and register our own middleware there for handling WP.com API requests. Has this been considered?

Could be arguably deemed a separate concern:

Get the adapter to work in current Calypso (this PR)

Add middleware to adapt to different APIs (depending on whether we're within Calypso, or producing a bundle that's supposed to run on a self-hosted site)

If we're confident that we can solve the latter given the tools that we have, we can tackle it separately.

tyxla · 2018-08-23T14:15:07Z

client/state/wp-data/calypso-wp-data-store.js

+		setPreference,
+		savePreference,
+	},
+};


I was also expecting to see a reducer here. Do you think it would make sense to migrate part of the existing reducer to wp.data as well? It would definitely contribute to having a better picture of how a complete example would look.

Making reducers explicit here might help untangle the state tree (relevant for code-splitting?). I guess we'd need to extend the adapter to enable us to attach its collected top-level reducers to Calypso's state tree?

I agree, but we'll need to add dynamic reducer functionality to the Calypso store as a pre-requisite. I'll start on another PR for that.

I was also expecting to see a reducer here.

I think it helps to look at it through the "interface vs implementation" lens. The module exports something that adheres to store interface, i.e. it has selectors and action. Reducer is a part of implementation. It's possible to have valid implementations that don't have any reducer at all.

tyxla · 2018-08-24T10:04:32Z

I'd love to get this merged so we can iterate on it and use it for other explorations, but also am a bit hesitant to let it be used in production so early, when we aren't sure that there won't be major changes to the implementation.

What do you think about adding it all under a feature flag, and leaving production untouched for now?

ockham · 2018-08-24T12:15:34Z

I'd love to get this merged so we can iterate on it and use it for other explorations,

Agree 👍

but also am a bit hesitant to let it be used in production so early, when we aren't sure that there won't be major changes to the implementation.

What do you think about adding it all under a feature flag, and leaving production untouched for now?

Hmm, is this worth a feature flag? Code like this https://github.com/Automattic/wp-calypso/pull/26838/files#diff-c5060c1cb159240a551bb8922dbb9743 isn't going to become more readable if we add two different ways of connecting to state.

What if we follow a pattern that we've adopted for other SDK related work? Merge the adapter (e.g. the stuff in client/state/wp-data/, but not the modified components, but rather use the adapter solely in client/gutenberg/extensions/ for now?

tyxla · 2018-08-24T12:51:08Z

What if we follow a pattern that we've adopted for other SDK related work? Merge the adapter (e.g. the stuff in client/state/wp-data/, but not the modified components, but rather use the adapter solely in client/gutenberg/extensions/ for now?

Sounds good to me 👍 as long as we:

Don't rely on the adapter for anything in production
Can use this work for further explorations and iterate on it

ockham · 2018-08-27T13:09:40Z

Before being able to merge this, I think we need to wait for Gutenberg to release package versions that include WordPress/gutenberg#9261 and WordPress/gutenberg#9266

@gziolo, would you do the honors? 😁

dmsnell · 2018-08-27T13:33:08Z

client/state/wp-data/calypso-wp-data-store.js

+	useCalypsoStore: true,
+	selectors: {
+		getPreference,
+		isFetchingPreferences,


one of my concerns here is that we'll end up in this file with a giant list of all of our selectors and action creators and this will pin our bundles at full bloat on the initial bundle

do you see a way to avoid this scenario?

can you put them in their own file and import with the asterisk? This is what we do in Gutenberg. We make all selectors and actions public by design.

it's more about the bundle size than the list of imports. if I'm going to use getPreference in a plugin I don't want to pull in all of the hundreds of selectors, reducers, and action creators from Calypso

also I want this work to benefit Calypso so we can split our data stores there too, so the state code loads via webpack dependencies management

If we use @wordpress/data interfaces to access the Calypso Redux store, the interface won't be one giant store with hundreds of actions and selectors. It will be divided into many @wordpress/data stores: calypso/preferences, calypso/posts, calypso/plugins, ...

The client will access the stores using the select and dispatch functions or using the withSelect and withDispatch HoCs:

withSelect( select => ( { pluginsList: select( 'calypso/plugins' ).getWPorgPlugins( { category: 'writing' } ) } ) );

That doesn't necessarily mean that calypso/plugins will be a separate Redux store. calypso/plugins is an interface that has some contract (actions and selectors) and the implementation has a lot of freedom on how exactly to fulfill that contract.

It's an open question if these interfaces make code splitting possible or not. I just wanted to point out that the "file with a giant list of all of our selectors" scenario is very unlikely to happen.

maybe I'm mistaken but I believe that the purpose of this PR was to explore a wrapper of all of Calypso's data code behind one store vs. rewriting each separate area of state into its own store, meaning that the purpose was to explore "one giant store with hundreds of actions and selectors."

?

Personally, I'd like to use this as an opportunity to break up the monolith of state we have in Calypso, but I'd like for us to be deliberate about where we draw those lines of separation. I think it could make sense for a collection of selectors to use the same store, if it's similar data. In the end, I'd like to see Calypso using a small number of stores where the division lines are clear. It may also make sense for more than one wp.data store to use the same state tree within Calypso, for performance reasons (i.e. we don't want to have 50+ redux stores)

In a9a0ebd I moved this to a store named 'calypso/preferences' within a calypso-stores directory with an index. I think this makes sense and will be much easier to split out from the shared state as well.

gziolo · 2018-08-27T13:40:54Z

Before being able to merge this, I think we need to wait for Gutenberg to release package versions that include WordPress/gutenberg#9261 and WordPress/gutenberg#9266

I want to do it this week, there are 3 remaining blockers to be resolved as we want to bump most of the packages with major increment:

Framework: Remove deprecations slated for 3.7 removal WordPress/gutenberg#9163 - a bunch of deprecations to remove
Packages: Replace usage of lodash with lodash-es WordPress/gutenberg#9374 - start using lodas-es instead of lodash (still not clear how to properly set it up)
Packages: Change how required built-ins are polyfilled with Babel 7 WordPress/gutenberg#9171 - improve the way we publish packages and stop using polyfills by default

ockham · 2018-08-27T21:46:35Z

client/components/root-child/index.jsx

+				<ReduxProvider store={ this.context.store }>
+					<CalypsoWPDataProvider calypsoStore={ this.context.store }>
+						{ content }
+					</CalypsoWPDataProvider>


I think this needs to go to client/layout instead, at least for the purpose of this PR. RootChild is mostly used by modals, whereas the color scheme picker (like most components) is a direct descendant of client/layout. I think that in this PR, withSelect() and withDispatch() are currently picking up a 'global' registry instance (inside @wp/data).

I think I got this to work so it picks up the correct registry in my PR, #26930.

Thanks for the tip. I'll adjust this one too.

Would it make sense to have the WP Data Provider in both places? Both here and in ReduxWrappedLayout?

Addressed in e9be45c

Would it make sense to have the WP Data Provider in both places? Both here and in ReduxWrappedLayout?

Eventually, we'll need that Provider at the top of all component hierarchies contain components wrapped in withSelect and/or withDispatch, but for the sake of this PR, wrapping only in ReduxWrappedLayout should be enough.

(We might end up writing a HOC that contains both the Redux and Calypso WP Data Provider, and wrap all component hierarchies in that.)

jsnajdr · 2018-08-28T11:53:48Z

client/state/wp-data/calypso-registry-plugin.js

+import { mapValues, without } from 'lodash';
+
+const calypsoRegistryPlugin = calypsoStore => registry => {
+	const namespaces = {};


The fact that the plugin needs to maintain its own registry strikes me as a bit cumbersome. The desired state of things would be that there is one registry that allows to register all types of stores that provide a required interface (i.e., expose selectors and actions), no matter what the implementation is.

The current @wordpress/data registry seems to favor only one particular implementation of a store and doesn't accept any other one.

Using @wordpress/data public API to select a store now has an unexpected result:

@import { select } from '@wordpress/data'; select( 'calypso' ).colorSchemePreference();

It will crash because the calypso store is not there: it's in another, private registry.

I don't recommend using the global select, dispatch, subscribe exported functions. These are mainly for the WP context and even there we're thinking about "deprecating" them (won't be easy) in favor of always relying on the registryobject (registry.select/dispatch/subcribe) and the HoCs.

Still, I believe that a single registry should be able to rule them all. Is there any obstacle that would prevent that? I noticed that the P2 integration needs to patch an existing store's resolver (i.e., something that's not public API) -- that would be harder with a registry that doesn't know about store's internals.

Yes, I think a single registry is capable of handing everything. Right now, we use the "default registry" in WordPress packages (and Gutenberg) which may drive Calypso to use the same default registry for now, but I'm hoping we can move away from the default registry in the future and each consumer would have to decide what to include in its registry.

import { createRegistry } from '@wordpress/data'; import coreStoreConfig from '@wordpress/core-data'; import editorStoreConfig from '@wordpress/editor'; import calypsoStoreConfig from '../calypso'; export createRegistry( { 'core': coreStoreConfig, 'core/editor': editorStoreConfig, 'calypso': calypsoStoreConfig, } );

This adds a wp.data plugin that uses the existing calypso state tree but pulls in selectors as a first step to being cross-platform.

This adds action dispatch handling for the wp.data calypso plugin to allow existing Calypso actions to be dispatched against the Calypso store.

This addresses a couple of PR comments about some optimization and additional commenting.

This adds the data provider to ReduxWrappedLayout in addition to RootChild

This moves the preferences selectors/actions from a `calypso` wp.data store to a `calypso/preferences` wp.data store, for the purpose of separating out these stores and organizing them better. This also lays the groundwork for separating out the monolithic state tree in calypso today.

This creates a `custom-store-plugin` which is more general-purpose than the previous `calypso-registry-plugin`. It allows any pre-existing redux store to be used for a given wp.data store instead of wp.data creating one itself. This allows for two things: 1. Stores can use enhancers and middleware, which is not possible in the default wp.data configuration. 2. wp.data stores can share redux stores. Note: This also creates an `internals-plugin`, which is necessary to avoid the reimplementing of internals within the `calypso-registry-plugin`. The `use` function of the `internals-plugin` will be submitted as a PR to core Gutenberg as well so perhaps it will not be necessary for long.

coderkevin · 2018-09-06T07:18:58Z

I adjusted this PR further to use more general-purpose plugins. I think it makes what is going on here more composable in the future (e.g. specific calypso-based wp.data stores can be moved away from the monolithic Calypso store one at a time. This should also support resolvers at this point as well.

I also rebased this onto the update/gutenberg-packages branch until that PR gets merged. This fixes the problem of the window global for SSR.

This switches the code back to the way it works in @wordpress/data because it's necessary to chain plugins.

This adjusts the plugin to not subscribe to the same custom store more than once when it's passed in as a custom store for more than one wp.data registerStore call.

This is to fix the CI errors on the PR that are complaining about the shrinkwrap not being in sync.

sirreal · 2020-11-27T10:53:52Z

Can this or parts of it move forward, or shall we close this?

simison · 2020-11-27T11:14:45Z

cc @jsnajdr in case you'd like to study this for your wp-data work.

coderkevin · 2020-11-27T14:52:55Z

I'm pretty far detached from this work at this point. However, if someone has questions or wants to discuss anything with me, just let me know and I'll try to remember as best I can.

coderkevin added the Data label Aug 23, 2018

coderkevin self-assigned this Aug 23, 2018

coderkevin requested review from ockham, dmsnell, tyxla, youknowriad, ehg, simison and jsnajdr August 23, 2018 04:05

This was referenced Aug 23, 2018

Data: Registry - check for window WordPress/gutenberg#9261

Merged

wp.data Overview WordPress/gutenberg#8354

Closed

coderkevin force-pushed the try/wp-data-calypso-plugin branch 2 times, most recently from 8e50377 to fe25663 Compare August 23, 2018 05:10

coderkevin changed the title ~~Data: Try wp.data with calypso state tree~~ SDK: Try wp.data with calypso state tree Aug 23, 2018

ockham mentioned this pull request Aug 23, 2018

withSafeTimeout: Unguarded window reference breaks SSR WordPress/gutenberg#9265

Closed

jsnajdr reviewed Aug 23, 2018

View reviewed changes

ockham reviewed Aug 23, 2018

View reviewed changes

tyxla reviewed Aug 23, 2018

View reviewed changes

alisterscott added [Project] SDK [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Aug 27, 2018

dmsnell reviewed Aug 27, 2018

View reviewed changes

ockham mentioned this pull request Aug 27, 2018

SDK: Try wp.data with calypso state tree "rewrite" #26930

Closed

ockham reviewed Aug 27, 2018

View reviewed changes

jsnajdr reviewed Aug 28, 2018

View reviewed changes

coderkevin force-pushed the try/wp-data-calypso-plugin branch from fe25663 to 20b3b95 Compare August 28, 2018 15:42

coderkevin added 6 commits September 6, 2018 02:05

state: Try wp.data with calypso state tree

4e0f7a6

This adds a wp.data plugin that uses the existing calypso state tree but pulls in selectors as a first step to being cross-platform.

state: Action handling for wp.data calypso plugin

4befbc1

This adds action dispatch handling for the wp.data calypso plugin to allow existing Calypso actions to be dispatched against the Calypso store.

state: wp.data adapter adjustments

1388482

This addresses a couple of PR comments about some optimization and additional commenting.

state: Add WP Data Provider to ReduxWrappedLayout

bcc4b32

This adds the data provider to ReduxWrappedLayout in addition to RootChild

coderkevin force-pushed the try/wp-data-calypso-plugin branch from 9c06368 to d7324b4 Compare September 6, 2018 07:16

coderkevin changed the base branch from master to update/gutenberg-packages September 6, 2018 07:16

coderkevin added 3 commits September 6, 2018 14:00

wp-data: internals-plugin, mutate registry in use

6d8a6cf

This switches the code back to the way it works in @wordpress/data because it's necessary to chain plugins.

wp-data: custom-store-plugin, adjust subscribe

90b204b

This adjusts the plugin to not subscribe to the same custom store more than once when it's passed in as a custom store for more than one wp.data registerStore call.

Update shrinkwrap

c5a4515

This is to fix the CI errors on the PR that are complaining about the shrinkwrap not being in sync.

simison removed the [Project] SDK label Jan 15, 2020

simison closed this Nov 27, 2020

simison deleted the try/wp-data-calypso-plugin branch November 27, 2020 11:14

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 27, 2020

SDK: Try wp.data with calypso state tree #26838

SDK: Try wp.data with calypso state tree #26838

Conversation

coderkevin commented Aug 23, 2018 • edited Loading

matticbot commented Aug 23, 2018

ockham commented Aug 23, 2018

ockham commented Aug 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsnajdr Aug 23, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ockham commented Aug 23, 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

tyxla commented Aug 24, 2018

ockham commented Aug 24, 2018

tyxla commented Aug 24, 2018

ockham commented Aug 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmsnell Aug 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented Aug 27, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coderkevin commented Sep 6, 2018

sirreal commented Nov 27, 2020

simison commented Nov 27, 2020

coderkevin commented Nov 27, 2020

coderkevin commented Aug 23, 2018 •

edited

Loading

ockham commented Aug 23, 2018 •

edited

Loading

jsnajdr Aug 23, 2018 •

edited

Loading

dmsnell Aug 27, 2018 •

edited

Loading