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

Packages: Singleton pattern prone to duplicate, distinct module scopes #8981

Open
aduth opened this issue Aug 14, 2018 · 6 comments

Comments

@aduth
Copy link
Member

commented Aug 14, 2018

Discussed today in the Core JavaScript meeting: https://wordpress.slack.com/archives/C5UNMSU4R/p1534251793000100
Originally raised at: Automattic/wp-calypso#26438 (comment)

An original issue was observed with @wordpress/data, where multiple registries could be simultaneously present due to independent versioning of packages having different dependencies upon the specific version of @wordpress/data.

Some observations include...

  • This is not specific to @wordpress/data, and could affect anything which behaves in a global / singleton fashion: @wordpress/hooks, @wordpress/filters, @wordpress/blocks
  • The issue is mitigated but not eliminated by moving away from Lerna independent versioning (toward fixed versioning) because the chance for differing versions still exists

Options for moving forward considered include...

  • Avoid "globals" behavior, where a consumer must define and operate with its own registry (specific to @wordpress/data)
    • How does this work in the WordPress context?
    • Does a default registry still exist? (specific to @wordpress/data)

cc @jsnajdr @gziolo @adamsilverstein @youknowriad

@coderkevin

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2018

Avoid "globals" behavior, where a consumer must define and operate with its own registry (specific to @wordpress/data)

Why not just always use registry-provider instead the global value? It already works. Also, setting the value of the provider is compatible with @wordpress/data plugins.

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2018

@coderkevin Yes this is one of the solutions on the table (for the data module at least) but it's not a simple switch:

  • How do plugins access the editor's state if it's not a global registry?
  • Should we expose registerBlockType (and similar) in the blocks package or should these be moved to WP specific context only?
  • What about the current usage of select,dispatch and subscribe in our current packages.

These have solutions (probably injecting these APIs using inline scripts and forbidding their usage in the packages themselves) but it's not smallish :)

@aduth

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2018

Yes, internally the data module operates on single registries, but also defines a "default" registry, which was specifically intended to improve usability by plugins in a WordPress context (by calling wp.data.select etc). It may be that the module should not define this registry and, if one is to still exist in the WordPress context, it must be defined by the WordPress build. Still an open question as to exactly how this looks. Are those methods still available at wp.data.select, or would this break expectations that wp globals mimic the API interface of their npm counterparts?

@coderkevin

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2018

So, if one uses a plugin and sets the value in the provider, wp.data.select will still map to the "default" registry, from what I'm understanding? If that's the case, it seems a bit broken, right? Plugins should be honored by default.

I am a bit biased as I tend to eschew globals whenever possible. But my preference is for reasons such as this, and for testing as well.

@aduth

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2018

So, if one uses a plugin and sets the value in the provider, wp.data.select will still map to the "default" registry, from what I'm understanding?

No, if a developer uses the provider, any descendents using select will leverage the registry specified by the provider. But a provider is not strictly necessary, in which case the default registry will be used. This is what occurs in Gutenberg.

<RegistryConsumer>
{ ( registry ) => (
<ComponentWithSelect
ownProps={ ownProps }
registry={ registry }
/>
) }
</RegistryConsumer>

See also #7453 as an example of providing a custom registry via provider to create an embedded editor experience.

@ockham

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2018

So, if one uses a plugin and sets the value in the provider, wp.data.select will still map to the "default" registry, from what I'm understanding?

No, if a developer uses the provider, any descendents using select will leverage the registry specified by the provider. But a provider is not strictly necessary, in which case the default registry will be used. This is what occurs in Gutenberg.

FWIW, I think this sort of confusion is quite natural with a double-edged (explicit/default) approach like this (RegistryProvider with explicit registry, vs otherwise falling back implicitly present global one). I was somewhat bitten by that while experimenting with Automattic/wp-calypso#26930 where it would use the global rather than the one I was explicitly providing (I think I was using a wrong prop name for a while, so the consumer would use the default registry).

I don't really have a horse in this raise, but I think the purported better developer ergonomics that the global/default registry is supposed to give is set off by the potential errors one makes when trying to migrate to the RegistryProvider-provided one. I'd vote to remove the global one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.