Skip to content
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

Remove wp.data plugin support #12004

Closed
wants to merge 3 commits into from
Closed

Remove wp.data plugin support #12004

wants to merge 3 commits into from

Conversation

coderkevin
Copy link
Contributor

@coderkevin coderkevin commented Nov 16, 2018

Fixes #11449

Description

With the support of generic stores, it becomes acceptable for the
default implementation (namespace-store) to contain what is now in
plugins. This functionality will be limited to namespace-store plugins
and will not be present when using registerGenericStore.

This PR builds the existing controls and persistence plugins into the namespace-store default implementation of stores for @wordpress/data. It also deprecates the and use function from the registry as they it will no longer be needed.

How has this been tested?

All existing tests pass.
Tested editor in Chrome and Firefox browsers (although browser support should not affect this PR).

Types of changes

This removes the use of plugins from internal @wordpress/data code and deprecates the use function.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@youknowriad
Copy link
Contributor

This is a great start, let's try to remove the persistence plugin too and add a deprecation warning.

With the support of generic stores, it becomes acceptable for the
default implementation (namespace-store) to contain what is now in
plugins. This functionality will be limited to namespace-store plugins
and will not be present when using `registerGenericStore`.
This moves the persistence feature out of a data plugin (which will be
deprecated) into the namespace-store default implementation of a wp.data
generic store. It also moves the tests into appropriate places and adds
new ones at the newly exposed functions.
With the two internal data plugins now built into namespace-store,
registry.use can be deprecated in preference of using
`registerGenericStore` for implementing custom behavior
@coderkevin
Copy link
Contributor Author

coderkevin commented Nov 20, 2018

@youknowriad I have moved the persistence plugin into namespace-store and added tests, and I also deprecated registry.use. This is ready for final review.

@coderkevin
Copy link
Contributor Author

The e2e test error looks like it has been happening randomly on unrelated builds as well, so I think it's a false failure. Can someone please retrigger the build? Thanks.

@youknowriad youknowriad changed the title WIP - Remove wp.data plugin support Remove wp.data plugin support Nov 26, 2018
deprecated( 'registry.use', {
alternative: 'registry.registerGenericStore',
plugin: 'Gutenberg',
version: '4.6.0',
Copy link
Contributor

Choose a reason for hiding this comment

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

We should remove the version here, we can't remove APIs anymore. We need to figure out the deprecation strategy first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what the next steps are for this now that it can't be removed. So whatever you want to do is fine.

' wp.data',
' .use( wp.data.plugins.persistence, { storageKey: storageKey } )',
' .use( wp.data.plugins.controls );',
' wp.data.setPersistenceDefaults( { storageKey: storageKey } );',
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about the naming of this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not a huge fan of this approach either, honestly. It's due to an artifact of the global registry. Feel free to rename or modify as you prefer.

@youknowriad youknowriad requested a review from aduth November 26, 2018 08:50
@aduth
Copy link
Member

aduth commented Nov 27, 2018

Hmm, I must have missed this part of the discussion in #10289 . It's not immediately obvious why we'd care to bake in all possible behaviors to the namespace store. I understand createGenericStore is always an option for a less opinionated interface, but I'm not convinced the pluggability is actually causing us issues? I quite like the spectrum of options available from strictly unopinionated to a full-featured store. For example, I might not have need for persistence in some project of my own, while still wanting the rest of the behaviors of a Redux-like store. It also provides a nice consistent interface for options, which serves in contrast to the uncertainty of the naming considered here.

@youknowriad
Copy link
Contributor

It's not immediately obvious why we'd care to bake in all possible behaviors to the namespace store. I understand createGenericStore is always an option for a less opinionated interface, but I'm not convinced the pluggability is actually causing us issues? I quite like the spectrum of options available from strictly unopinionated to a full-featured store. For example, I might not have need for persistence in some project of my own, while still wanting the rest of the behaviors of a Redux-like store.

I agree that you should opt-in into persistence and controls, it is still possible right now because these are options you pass to the store. But I think I find it confusing to allow .use Extensibility but we know at the same time that it works for a just one kind of stores (namespaces store). For me, the .use API is conflicting with the createGenericStore API and plugins built with this API can't work across stores because they are based on implementation details of the namespaces stores. I strongly believe we should deprecate .use for these reasons.

@youknowriad
Copy link
Contributor

I'm not sure how to express it but even if I liked the .use API initially for what it allowed, even without createGenericStores, I think the plugins implemented showed that it's an API that's not well defined. you basically can attach anything to the registry object and can also tweak anything from the registry object but you must know the internals of these function to understand how to extend them properly.

@gziolo
Copy link
Member

gziolo commented Feb 5, 2019

@aduth and @youknowriad - do we still want to deprecate plugin support given that it was included in WordPress 5.0 release? By the way, this plugin got stale and would have to be refreshed with the latest changes from master branch.

@gziolo gziolo added [Package] Data /packages/data [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. labels Feb 5, 2019
@aduth
Copy link
Member

aduth commented Feb 5, 2019

I vehemently disagreed with this one from the beginning 😄

@youknowriad
Copy link
Contributor

I still think there's an inconsistency in our current code base. This API is only useful for "namespace stores" but it's defined by default in the registry object for all stores while it doesn't apply for any other implementation.

I still think this is a duplicate of the createGenericStore API but a bit less elegant as it forces us to expose the internals of the namespace stores as an API.

My thinking is that we can remove it at the moment but at least we should clarify that it's not the way we should add behavior to stores.

@aduth
Copy link
Member

aduth commented Feb 11, 2019

@youknowriad As I look to implement what I'd described in #13088 (comment) , my intuition led me to think that "plugins" were the best pattern we had to align with an implementation for history. I'm not sure I follow the exact concerns you laid out in your previous comment, where to me, the namespace-stores were what we assumed to be the default / preferred implementation, and "generic" exposed as a niche side-case. Your prior comment makes it sound as though we optimize for the latter, though. And in which case, what alternative option would you consider for adding behaviors to stores, and particularly for those which might operate across stores (like history would). The editor middlewares approach is what I see as our other alternative here, but this is in its own way a very inelegant approach, at least if it were to be implemented from scratch for each store.

@aduth
Copy link
Member

aduth commented Feb 11, 2019

Reducer-enhancers / higher-order reducers is another pattern we have, particularly relevant to this discussion since it's how we implement history today. The ideas in #13088 (comment) still pose a unique challenge in how we manage history across stores, though it seems like something where a global shared state could be appropriate (or at least per-session "global", as in a top-level registry, considering server-side implications if it's something to support).

@aduth
Copy link
Member

aduth commented Feb 11, 2019

Related previous note about the idea for a module of reducer-enhancers: #9617 (comment)

  • I copied onSubKey higher-order reducer creator from core-data. I'm starting to wonder if we might want to consider distributing these as some form of data/reducer utilities, akin to as @wordpress/compose is for higher-order components. Maybe a @wordpress/compose-data or @wordpress/compose-reducer.

@youknowriad
Copy link
Contributor

where to me, the namespace-stores were what we assumed to be the default / preferred implementation, and "generic" exposed as a niche side-case.

I don't see it this way, for me the namespace store is just another implementation of a store (it could be an extension itself as createNamespaceStore could in theory be moved out of the data module and just rely on createGenericStore). For me the low-level API is the main API of the data module and it is the generic stores because it encompass all kind of stores which mean if one would build a plugin for the registry, it should support all kind of stores and not one specific type.

Right now, when you do wp.data.use( persistence ), it's not clear that this plugin is only supported by a specific kind of store (namespace) which itself could not be part of the data module entirely.

@aduth I think we need to distinguish two things here:

1- The idea that when you use the namespace store, you can opt-in to features like persistence, history, controls... I'm totally ok with this idea, I think it's good. What I'm not confident with is the way we add those features today.

2- The .use function which is in theory a plugin mechanism to enhance the registry but in our case it's a mechanism to enhance the namespace store.

Also, I think after using this API, I'm not a fan of it because it's not clear what you can and can't do with it, you need to read the namespace store implementation to know how you can extend it. The way you write these plugins is very tied to the original implementation of the namespace store.

@gziolo gziolo added the Needs Decision Needs a decision to be actionable or relevant label Feb 13, 2019
@gziolo
Copy link
Member

gziolo commented Apr 24, 2019

I discussed this PR with @coderkevin in private chat. I think we should close this issue for now as he is not going to work on it until we have a clear path how to proceed further. As soon as there is an agreement on the final shape of this proposal, he is happy to reopen this PR and help to land it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Decision Needs a decision to be actionable or relevant [Package] Data /packages/data [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate registry.use() and update internal implementations
4 participants