Data module: Add built-in support for persisting stores

[youknowriad]

Related #8081

This PR adds a built-in mechanism to the data module to persist the registry.

It works this way:

• When registring a store that you want to persist, pass a persist: true to the registerStore function call.
• To trigger persistance and initial loading you just call registry.setupPersistence( storageKey )

This allows us to persist the entire registry in one call instead of spreading this call to multiple packages. It also allows us to avoid depending ono window.userSettings in several packages to generate the unique uid for persistence.

Testing instructions

• Open Gutenberg
• Close the sidebar
• Reload the page
• The sidebar should stay closed.

[gziolo]

Conceptually this is a very nice improvement. I prefer this new API over what we had before. In particular, it turns out to be common use case so this is highly expected simplification for the developer's convenience.

I left a few notes which needs to be addressed. There might be one place where we need to optimize the way we check if the previous state didn't change.

I still see one place for improvement. In this code:

const store = registerStore( 'core/edit-post', {
	reducer: restrictPersistence( reducer, 'preferences' ),
	actions,
	selectors,
	persist: true,
} );

I feel that there is some duplication since we use restrictPersistence helper, we are already expressing our intent quite clearly so there shouldn't be a need to enable persist flag, too. I would personally seek a way to enable this flag behind the scenes using the aforementioned helper. Some alternatives I could propose are:

// one subreducer persisted
const store = registerStore( 'core/edit-post', {
	reducer: withPersistence( reducer, 'preferences' ),
	actions,
	selectors,
} );
// everything persisted
const store = registerStore( 'core/edit-post', {
	reducer: withPersistence( reducer ),
	actions,
	selectors,
} );

[gziolo]

STORAGE_KEY should be removed, it's no longer necessary in here.

[gziolo]

It looks like this check will always be true. reduce will always return a new object so this comparison needs to be updated. Should it use isShallowEquals instead? It would be nice to add unit test which ensures it works properly even when I'm wrong :)

[youknowriad]

Definitely a shallow equal :)

[gziolo]

Should we put it inside initializeEditor just before core blocks get registered?

[youknowriad]

I don't know, I put it here to keep it close to the previous behavior. The stores are created when the scripts are loaded, so I thought the stores should be initialized (loading the initial value) once the stores are loaded as well.

[gziolo]

yeah, it probably doesn't matter that much

[gziolo]

This is breaking change. I think we should expose copies of loadAndPersist and withRehydration with deprecation warning informing about alternatives.

[youknowriad]

Definitely, I'll bring them back, since we didn't really document those, I think their usage is close to zero but let's deprecate though :)

[gziolo]

It would be nice to have it located in the file next to the setPersistenceStorage method.

I also think that since we introduce this method we could do a very tiny refactor:

let persistenceStorage;

export function getPersistenceStorage() {
    return persistenceStorage || window. localStorage;
}

to ensure it never tries to reference window when you set persistence when doing setup. I think at the moment it might error in the env where window is not present.

[youknowriad]

I feel that there is some duplication since we use restrictPersistence helper, we are already expressing our intent quite clearly so there shouldn't be a need to enable persist flag, too. I would personally seek a way to enable this flag behind the scenes using the aforementioned helper. Some alternatives I could propose are:

In this PR, there's a difference between the way persistence work now and the way it worked before. In the previous implementation, persistence always had to persist a subkey of the store while by default it persists the whole store now if you enable it, which means restrictPersistence is not about enabling persistence, it's only about restricting the persistence to subkey of the store, it's a helper function. It's definitely possible to support persistence while not restricting it to a single subkey. That's the reason I think, it shouldn't be named withPersistence.

Also, Persistence impacts the whole store and not only the reducer, so I thought a separate flag when registering the store made more sense to me.

Thanks for the review, it should be addressed now

[gziolo]

Any ideas why Travis is sad?

[youknowriad]

@gziolo We had a "hidden" issue with our withHistory higher-order reducer, it was not creating a pure function. So if we use our reducer in two different places (stores), it creates conflicts. It should be fixed with my last commit. (This could probably also fix some undo/redo open issues)

[gziolo]

We had a "hidden" issue with our withHistory higher-order reducer, it was not creating a pure function. So if we use our reducer in two different places (stores), it creates conflicts. It should be fixed with my last commit. (This could probably also fix some undo/redo open issues)

I can see withHistory usage only inside editor store, does it mean we instantiate it twice?

[gziolo]

Just noting, that we are changing the existing logic for all action types. In the past, we would assign action instead. Isn't it what caused the issue rather than the fact that we were using instance variables?

I prefer to have them stored as they are in the updated version but I'm trying to understand what was the original issue.

[youknowriad]

Yes, I know but this is a small change that doesn't have any impact but I think it's better this way because when you click the "undo/redo" you don't want to perform a comparison with these actions later, instead, you want to just record a new "past" when you start editing again

[gziolo]

In my testing, everything works as expected. I can see all values properly persisted in the local storage.

I'm still not quite sure why those tests were failing on Travis, but with the changes introduced, I can't reproduce them on my local machine.

It would be awesome to have some more eyes on this PR but I wouldn't wait too long as it blocks #8081 where we want to turn editor module into NPM package.

[youknowriad]

I'm still not quite sure why those tests were failing on Travis, but with the changes introduced, I can't reproduce them on my local machine.

The issue is that we were using the reducer to serialize the content but also as the reducer for the store but since the reducer was not "pure", those local variables were conflicting between those separate calls of the reducer.

[aduth]

This feels† like a strange thing to build in to the data module. I'd rather we had exposed the option to add enhancers easily, then packaged the persistence into a separate module to reuse. This way, the core of the data module could remain lean, while enabling pluggability of various functionality (vs. having some mega-capable monolith package).

† Note: I've intentionally avoided looking too deeply into the reasons why it was done this way, so as to make it apparent this is an observation from the uninitiated.

[aduth]

Not really loving how this API is evolving. At a glance, I have no idea what false is in the following line of code:

wp.data.registerReducer( 'core/my-store', reducer, false );

If we're going to add additional options, maybe we should just have this be part of an options object. At least that way, it'll be clearer what we're doing with named keys, and allow room to grow, where order of the arguments is already not important.

[youknowriad]

I don't like it a lot as well but I consider these register* functions as internal functions that should be deprecated at some point in favor of registerStore (especially registerReducer) and the registerStore function already has the options object.

[gziolo]

Yes, I had also some concerns regarding this flag, but we solved a bunch of other issues. I agree that an object would be easier to read.

[aduth]

This way, the core of the data module could remain lean, while enabling pluggability of various functionality

As I return to #8096 which adds built-in support for a "controls" concept, I recognize the irony in my own statement. I think ultimately we'll want some judgment in what sorts of features are appropriate to include in the core of the data module. Persistence struck me as perhaps not quite fitting, though it's fair to acknowledge a similar argument could be made about asynchronous controls (or resolvers, etc).

And maybe this is valid. In #8096, it's already distributed as a plain Redux middleware, so it wouldn't be hard to revert back to suggesting developers integrate this way.

[youknowriad]

Good observations @aduth. The original goal of this PR is to separate the support for persistence in stores from the activation of the persistence into a specified key (unique user key), which would make it possible to build a registry using several stores and decide to persist it at a later point (or not) to a given key. This helps in making editor and nux generic npm packages.

Yes, a global enhancer API would make sense but it's very hard to design an extensibility mechanism for the registries (not per store). I'm fine building some features in now (controls, persistence, even resolvers maybe) which hopefully would inform the way we should develop the Registry Enhancer API.