Data: Add persistence via data plugin interface #8341

aduth · 2018-08-01T00:28:40Z

In Progress: Functionally working. Lacks tests, inline code documentation, and deprecations.

This pull request seeks to explore a plugin interface for data registries, refactoring persistence as a separate module which can optionally be pulled into a data registry via the new use function.

A plugin is defined as a function which receives the registry object, and is expected to return an object of function overrides. For persistence, the plugin is created with a storage key, and returns an object overriding the registerStore function of a registry to inject persistence behaviors.

Prior art:

Example:

var storageKey = 'WP_EDIT_POST_DATA_' + window.userSettings.uid;
var plugin = wp.dataPluginPersistence.createPersistencePlugin( storageKey );
wp.data.use( plugin );

youknowriad · 2018-08-01T09:13:02Z

packages/data/src/registry.js

@@ -377,6 +326,21 @@ export function createRegistry( storeConfigs = {} ) {
 		subscribe,
 		select,
 		dispatch,
-		setupPersistence,
+		namespaces,
+		use,


So basically, the API changes are:

making the namespaces array a public API.

I think it's fine.

The issue we'll have is how to update the default registry since it's not mutable. The use generates a new registry? Should we have a "special" use function for the default registry?

It doesn't so much create a new registry as much as its own internal representation. withPlugins effectively makes what is exposed behave as a proxy to the current internal representation. It was partly done this way so that plugins act as layers, where the registry it receives when called is either the original implementation, or whatever was last overridden by a plugin.

making the namespaces array a public API.

Yes, I wasn't so sure about this. I still think it could be possible to omit, or limit (to e.g. an array of the registered reducerKey).

Since registerStore returns the actual store instance, at the worst case the plugin overridden value could inject the triggerPersist as part of the store's own dispatch / getState.

I missed withPlugins, clever :)

Sometimes I wonder if we should "deprecate" the use of the global registry entirely but that's a different subject.

I think it's a nice convenience to have. As made evidenced here, it does make some things more difficult, like the idea of a "global plugin", which must be injected before anything else creates a store of its own (the wp_add_inline_script on "NUX" is not super obvious or easy to maintain).

youknowriad · 2018-08-01T09:14:00Z

packages/data/package.json

@@ -25,7 +25,6 @@
 		"@wordpress/compose": "file:../compose",
 		"@wordpress/deprecated": "file:../deprecated",
 		"@wordpress/element": "file:../element",
-		"@wordpress/is-shallow-equal": "file:../is-shallow-equal",


I think it's still used in the module (withSelect)

gziolo · 2018-08-01T09:16:10Z

packages/nux/src/store/index.js

 	actions,
 	selectors,
-	persist: true,
+	persist: [ 'preferences' ],


I like it 👍

gziolo · 2018-08-01T09:17:29Z

packages/data-plugin-persistence/package.json

@@ -0,0 +1,29 @@
+{


We probably reach the point where we should create a script which scaffolds new package 😛

It's quite prone to copypasta as-is 😄 🍝

youknowriad · 2018-08-01T15:42:08Z

lib/client-assets.php

 		filemtime( gutenberg_dir_path() . 'build/nux/index.js' ),
 		true
 	);
+	wp_add_inline_script(
+		'wp-nux',
+		'wp.data.use( wp.dataPluginPersistence.createPersistencePlugin( "WP_EDIT_POST_DATA_" + window.userSettings.uid ) );',


Should we do the same in the editor module? or should we leave this for edit-post only?

After discussion, I guess we need to do this before the store registration so probably as an inline script for wp-data and we should rename the key in that case 'WP_DATA_' + userId

aduth · 2018-08-01T23:50:41Z

Pushed a number of updates:

Integrated persistence as a plugin of the @wordpress/data module, enabled by default for the default registry
Moved persistence initialization as an after inline script of the wp-data script handle, renaming the localStorage key (and providing transitional backwards-compatibility)
Restored deprecations and included deprecation path for restrictPersistence and setupPersistence
- This isn't 100% backwards-compatible in that it won't respect SERIALIZE and REDUX_REHYDRATE custom behaviors
Add documentation
Add tests
Fix error in Private Browsing for Safari 10 and earlier (currently present in master) with an "object storage" fallback to localStorage (also used in unit tests)
Add support for both enhancers and middlewares options on registerStore (closes Allow custom stores to set custom enhancers (middlewares) #7417)
- The latter is simply a convenience for the former
Removed exposing of namespaces (reimplemented on persistence plugin as a middleware)

The one limitation I see in the current implementation is that since the default set of plugins are "global", while registries can choose to opt in or not, and the plugins could operate on a per-registry basis, there's no way to distinguish by registry for the purposes of persistent storage. Therefore, there's the potential for conflicts if two registries both enable persistence and have reducers with the same reducerKey. I sense it's this way in master as well, so I'm not sure it's too much to concern over.

youknowriad · 2018-08-02T08:54:23Z

packages/data/src/plugins/persistence/README.md

+Call the `use` method on the default or your own registry to include the persistence plugin:
+
+```js
+wp.data.use( 'persistence' );


Can I ask why you preferred the "string" syntax over the "function" one (similar to redux).

I prefer personally wp.data.use( createPersistencePlugin( { storage: window.localStorage, key: 'myKey' } ) )

for these reasons:

No global state which means no conflict between registries

Ability to create custom plugins easily without a need for a addPlugin/registerPlugin API.

It doesn't strictly need to be a string. It's this way largely as a convenience. It could be a pointer to the function as well:

wp.data.use( wp.data.plugins.persistence );

Part of the thinking was consistency between plugin initialization. Considering others that we may explore to refactor as plugins (resolvers, controls), they don't require such "creation".

So it would be odd to have both wp.data.plugins.persistence.createPlugin and then just wp.data.plugins.resolvers (or a wp.data.plugins.resolvers.createPlugin that accepted no arguments).

And there already is one layer of creation with the plugin called as a function with the registry as an argument. It's possible we could have options as an optional argument to the use, so we have something like:

wp.data .use( wp.data.plugins.persistence, { storageKey: '...' } ) .use( wp.data.plugins.resolvers );

No global state which means no conflict between registries

It's still not very clear to me the best way we want these "default" plugins to be brought in. If we want this to occur within data itself, it seems reasonable that there's some global context; how else would we customize the storage behavior without the storage key (with user ID) being known to the data module (bad for generalism)?

I'd also thought it might be suitably explicit if the modules which register stores would always make sure to call use before calling to register their own store.

use( persistence ); registerStore( 'core/edit-post', { /* ... */ } );

This would require some level of deduplication of plugins, though it's easy enough to achieve.

createPlugin is the responsibility of the plugin author, I'm mostly concerned about reducing the API surface as much as we can. If we have just registry.use( pluginFunction ) I'm fine with that.

I can't see the downside of this, it just allows everything:

configuration using createPlugin like function returning the plugin function

non-configurable plugins

It's still not very clear to me the best way we want these "default" plugins to be brought in. If we want this to occur within data itself, it seems reasonable that there's some global context;

I think we should limit the global context as much as we can.
If we do:

wp.data.use( wp.data.persistence.createPlugin( config ) )

in an inline script, we have both the defaut plugins built-in and also no globals and no leaking to other registries. If you want to customize the storage engine, you do it in that call above. Maybe I'm missing something here?

I'd also thought it might be suitably explicit if the modules which register stores would always make sure to call use before calling to register their own store.

For me, that's probably the most compelling reason for the globally available plugins use( 'persistence' ) but even though, I'm not certain we can make it work without having plugins declared by a store messing up with plugins declared by other stores.

The main remaining piece is consistency between including multiple plugins. I would not like, for example:

wp.data .use( wp.data.plugins.persistence.createPlugin( config ) ); .use( wp.data.plugins.resolvers );

...because this breaks expectations on how a developer works with a specific plugin (need to understand whether it has a createPlugin or not).

That's where I'm thinking of either:

wp.data .use( wp.data.plugins.persistence, { storageKey: '...' } ) .use( wp.data.plugins.resolvers );

Where the object effectively causes the plugin to be "created" in the same way you've mentioned. This would look like:

function plugin( registry, options ) { // ...initialize storage from options, available to closure }

Alternatively, we could assume that use should be called with a created plugin, but have the developer pass the registry, in place of current implementation of use calling with registry as an argument automatically.

wp.data .use( wp.data.plugins.persistence.create( registry, { storageKey: '...' } ) .use( wp.data.plugins.resolvers.create( registry ) ); // Or: With default registry? // .use( wp.data.plugins.resolvers.create() );

wp.data
.use( wp.data.plugins.persistence, { storageKey: '...' } )
.use( wp.data.plugins.resolvers );

This one has my vote then.

I'm thinking most of these make sense anyway, some small differences but in the end it's the same thing. So let's move forward with what you think is best.

Shorter is better, second that 😃

youknowriad · 2018-08-02T08:56:13Z

packages/data/src/registry.js

+			options.persist = [ options.reducer.__keyToPersist ];
+		}
+
+		if ( options.middlewares ) {


I'm personally not certain we'd like to expose middlewares/enhancers because these are very highly tied to Redux and I know there ware discussions about changing enhancers API in future versions of Redux.

Any particular reason for including this in the current PR?

I know there ware discussions about changing enhancers API in future versions of Redux.

Do you have a link for more information?

Enhancers aren't too much more than what we already enable by returning the store from registerStore. We're using this ourselves to apply middleware manually, so it's obviously a requirement already.

The main goal here was avoiding the need to expose namespaces, which is an even deeper implementation detail that I'd worried to expose. Authoring as a middleware was a means to reimplementing persistence to avoid depending on it, as an alternative to checking whether state has changed after a dispatch. That said, it could also be done with subscribe now that I think of it.

I don't feel too strongly either way, but I also don't think middlewares / enhancers are too "internal" or "Redux"-y to warrant hiding; that said, I'm curious about the discussion you refer to. 😄

Found the discussion I was talking about reduxjs/redux#1702

Skimming the discussion there, I can see how most everything could be implemented on a base of getState and dispatch. I even think in the pull request there that it could be taken further:

onChange is an unnecessary part of storeBase which can be applied as composed in overriding dispatch.

initialState could be avoided, assigned in the reducer as the default value for the state argument.

This loses parallels to Array#reduce, but I assume an intention with initialValue there is in allowing intentional accumulation of undefined, which is forbidden in Redux.

There's an open question of convenience and that these patterns are already well-established in the Redux ecosystem.

For the purposes of this pull request, I can see whether it's easy enough to avoid the enhancers by just overriding dispatch on the registered store instance.

aduth · 2018-08-02T17:36:00Z

More updates:

Default registry no longer initializes default set of plugins
Changed plugin signature to allow accepting options
wp-data inline script initializes persistence plugin
Removed support for middlewares / enhancers options from registerStore
- Reimplemented persistence as a flow on store.dispatch
Removed support for use with a string argument

To do:

Verify deprecation behaviors (e.g. pretty sure the __keyToPersist behavior won't be working as it appears it should in registry.js)
Unit tests for use

aduth · 2018-08-02T20:27:53Z

I think this is ready for a final review.

gziolo

It looks great. I like how it evolves and make API more general. I had only one comment about assumption we make about persistence storage which might be too strict.

@youknowriad, how do you feel about refactoring apiFetch package to follow the pattern established in this PR in regards to use interface?

I think an example in the description is outdated and might be misleading for some people trying to build their own plugins.

gziolo · 2018-08-03T04:02:23Z

lib/client-assets.php

+		implode( "\n", array(
+			// TODO: Transferring old storage should be removed at v3.7.
+			'( function() {',
+			'	var userId = window.userSettings.uid;',


This simplifies a lot in terms of passing user id to the client 👍

gziolo · 2018-08-03T04:10:47Z

packages/data/src/plugins/persistence/storage/object.js

@@ -0,0 +1,23 @@
+let _objectStorage;


Do we have usage of _ as a prefix documented? I did a quick search and found only one place where we use let _variable. With ES6 modules it doesn't seem to be so important to use this patter to indicate that variable is private.

I agree. I'll remove the _ prefix.

gziolo · 2018-08-03T04:16:57Z

packages/data/src/plugins/persistence/index.js

+				data = {};
+			} else {
+				try {
+					data = JSON.parse( persisted );


Should we make JSON handling optional? It might be very limiting for some type of storages like IndexedDB or WebSQL. What about async?

Definitely not a blocker. However, this immediately raised such questions.

It's an interesting point. Asynchronous storage is probably desirable anyways, or at least it could serve as a useful proof-of-concept for how a plugin might initially asynchronously. I'll explore it a bit.

So a problem here is that we need this data available at the time of initializing the store, meaning that if it must be fetched asynchronously, we'd have to introduce some API for blocking the registration of a store until said async behaviors resolve. I don't think this is something we'd want to allow. Instead, I might imagine the better alternative would be for the consumer to do the asynchronous fetching themselves as a step before they call to register the store, then surface it to the persistence plugin by the storage option. This doesn't need to be localStorage specifically, but it does need to be a synchronous storage mechanism (at least for retrieval) that could interface on top of the previously-asynchronously-fetched indexedDB data, for example.

setItem for this plugin could be considered a "fire and forget" operation that could be implemented asynchronously if desired.

Oh, I overlooked a main point here in the mention of JSON as the value storage mechanism. I think as long as we want this to behave as an implementation of the Web Storage API interface, we should similarly respect its mechanics, i.e. setItem takes a value which is a string (more specifically, DOMString):

https://developer.mozilla.org/en-US/docs/Web/API/Storage/setItem
https://html.spec.whatwg.org/multipage/webstorage.html#the-storage-interface

gziolo · 2018-08-03T04:27:35Z

packages/data/src/plugins/persistence/test/index.js

+
+		registry.registerStore( 'test', {
+			reducer,
+			persist: true,


So true is still possible. Good.

gziolo · 2018-08-03T04:37:31Z

This isn't 100% backwards-compatible in that it won't respect SERIALIZE and REDUX_REHYDRATE custom behaviors

Verify deprecation behaviors (e.g. pretty sure the __keyToPersist behavior won't be working as it appears it should in registry.js)

We definitely should do a major release of the package when it lands. Can we also create CHANGELOG.md file so we could simplify the release process? We should have it documented... preferably using the patterns established by conventional commits so we could start using it at the same time.

I found this, it should work per package. I will set it up soon: https://michaljanaszek.com/blog/lerna-conventional-commits.

Update: Opened PR #8415 so we could discuss it there.

aduth · 2018-08-03T13:26:09Z

We definitely should do a major release of the package when it lands.

Should we? Here's the specific wording from the SemVer Specification (emphasis mine):

Major version X (X.y.z | X > 0) MUST be incremented if any backwards incompatible changes are introduced to the public API.

https://semver.org/#spec-item-8

This was never part of a public API.

aduth · 2018-08-03T13:27:29Z

Depending on the interpretation, I could see this being considered part of a "public API", yet not documented.

I've no issue with bumping the major. My main question would be how we deal with deprecations as they relate to breaking versions. In other words, do we need to bump the major again when we finally remove the compatibility shims?

Edit: Added an item to the Core JS Meeting agenda

aduth · 2018-08-03T13:37:35Z

Another feature we probably want for plugins is the ability to add methods to the registry API. For example, if resolvers are extracted as a plugin, we might want its registerResolvers to be available on the registry object. With the current implementation, this won't work, because while the function would be added to the internal representation of registry, the value returned by createRegistry never changes.

I'd be fine if this were addressed separately.

youknowriad · 2018-08-03T13:46:59Z

I think there's an easy way to decide to bump the major version or not right now. When we remove deprecations, we know the affected packages need to have a major version bump.

youknowriad

I'm approving, this is a great API that needs to be included and iterated on if needed.

aduth · 2018-08-03T15:41:57Z

When we remove deprecations, we know the affected packages need to have a major version bump.

Yeah, that was my initial thought. The worry for me was that we have two major versions we'd be bumping by this one change if we need to bump it for the fact that the deprecation shim is not 100% compatible, then again when the deprecated feature is removed.

Should (a) return null if not set, (b) assign value as string

1. Wrong property accessed used 2. Reducer is already wrapped at point registerStore is called. Detect from plugin instead.

Not conventional

aduth · 2018-08-03T18:23:03Z

Another feature we probably want for plugins is the ability to add methods to the registry API.

Maybe not, actually. In my resolvers example, this is something we should be encouraging developers to opt-in as an option of registerStore, not by calling registerResolvers on the registry directly.

That said, depending on how far we want to take this, something like select would need to be added as a method to the registry itself if selectors are to be reimplemented as a plugin (uncertain).

Anyways, I'm going to give this one more pass and then merge. Thanks for the reviews! 👍

aduth · 2018-08-03T18:33:26Z

FYI as a result of the discussions here, I opted to close #7417 as "wontfix". Feel free to follow-up there if you have thoughts.

aduth added [Status] In Progress Tracking issues with work in progress [Package] Data /packages/data labels Aug 1, 2018

aduth requested review from youknowriad and gziolo August 1, 2018 00:28

youknowriad reviewed Aug 1, 2018

View reviewed changes

gziolo reviewed Aug 1, 2018

View reviewed changes

packages/nux/src/store/index.js

actions,

selectors,

persist: true,

persist: [ 'preferences' ],

Copy link

Member

gziolo Aug 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it 👍

gziolo reviewed Aug 1, 2018

View reviewed changes

youknowriad reviewed Aug 1, 2018

View reviewed changes

aduth force-pushed the add/data-registry-plugins branch from f380a13 to ba58f0a Compare August 1, 2018 23:44

youknowriad reviewed Aug 2, 2018

View reviewed changes

aduth removed the [Status] In Progress Tracking issues with work in progress label Aug 2, 2018

gziolo reviewed Aug 3, 2018

View reviewed changes

gziolo added this to the 3.5 milestone Aug 3, 2018

youknowriad approved these changes Aug 3, 2018

View reviewed changes

aduth mentioned this pull request Aug 3, 2018

Allow custom stores to set custom enhancers (middlewares) #7417

Closed

aduth added 3 commits August 3, 2018 13:36

Data: Add persistence via data plugin interface

17a9f51

Data: Integrate persistence as first-party plugin

27c1ba3

Data: Restore deprecated persistence behaviors

03e4cb4

aduth added 11 commits August 3, 2018 13:36

Data: Return default persisted by caught error

f0f8c07

Data: Initialize persistence by plugin options

0b6c37f

Data: Remove string form variation on use

2dacdf1

Data: Refactor persistence as dispatch enhancer

4219594

Data: Remove middlewares / enhancers support for store

f00c9f5

Data: Remove unused variable

b3d13dc

Data: Improve objectStorage spec compliancy

2085eed

Should (a) return null if not set, (b) assign value as string

Data: Fix persistence handling of null value

94a1f4c

Data: Fix deprecation key to persist

8f8c544

1. Wrong property accessed used 2. Reducer is already wrapped at point registerStore is called. Detect from plugin instead.

Data: Add tests for WPDataRegistry#use

4f630b1

Data: Remove _ prefix from local variable

e4a1c0e

Not conventional

aduth force-pushed the add/data-registry-plugins branch from 37d3cd0 to e4a1c0e Compare August 3, 2018 17:37

Data: Update plugins documentation to avoid mention of string usage

fdc8938

aduth merged commit c1c4c7e into master Aug 3, 2018

aduth deleted the add/data-registry-plugins branch August 3, 2018 18:41

aduth mentioned this pull request Aug 16, 2018

Testing: Idea for E2E runtime store unhandled action mutations #9061

Open

aduth mentioned this pull request Sep 18, 2018

Persistence data plugin should take into account the object shape #9994

Closed

dependabot-preview bot mentioned this pull request Dec 31, 2019

Bump @wordpress/i18n from 2.0.0 to 3.7.0 wearerequired/rest-likes#97

Closed

dependabot-preview bot mentioned this pull request Feb 4, 2020

Bump @wordpress/jest-console from 3.4.0 to 3.5.0 tdreyno/druyan#215

Merged

dependabot-preview bot mentioned this pull request Apr 15, 2020

Bump @wordpress/babel-plugin-makepot from 3.4.0 to 3.5.0 swissspidy/gutenberg-i18n-block#93

Merged

Data: Add persistence via data plugin interface #8341

Data: Add persistence via data plugin interface #8341

Conversation

aduth commented Aug 1, 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

youknowriad Aug 1, 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

aduth Aug 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented Aug 1, 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

youknowriad Aug 2, 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

aduth commented Aug 2, 2018

aduth commented Aug 2, 2018

gziolo 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented Aug 3, 2018 • edited Loading

aduth commented Aug 3, 2018

aduth commented Aug 3, 2018 • edited Loading

aduth commented Aug 3, 2018

youknowriad commented Aug 3, 2018

youknowriad left a comment

Choose a reason for hiding this comment

aduth commented Aug 3, 2018

aduth commented Aug 3, 2018

aduth commented Aug 3, 2018

youknowriad Aug 1, 2018 •

edited

Loading

aduth Aug 1, 2018 •

edited

Loading

aduth commented Aug 1, 2018 •

edited

Loading

youknowriad Aug 2, 2018 •

edited

Loading

gziolo left a comment •

edited

Loading

gziolo commented Aug 3, 2018 •

edited

Loading

aduth commented Aug 3, 2018 •

edited

Loading