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

@wordpress/data: Export entire registry context #15445

Closed
wants to merge 3 commits into from

Conversation

nerrad
Copy link
Contributor

@nerrad nerrad commented May 5, 2019

Description

As a part of the work being done on #15444, the useContext hook is being used to obtain the registry value from the context. However in order to do this the entire registry context object must be exposed from @wordpress/data. This pull does so.

This has utility for anywhere else where useContext might be implemented with the registry context.

How has this been tested?

Types of changes

This is a non-breaking change and is an enhancement exposing the entire RegistryContext object. Existing Consumer and Provider values are left alone so there's no impact to existing code.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

nerrad added 2 commits May 5, 2019 19:30
This is needed for any consuming code that wants to implement the `useContext` hook for the registry.
@nerrad nerrad added [Type] Enhancement A suggestion for improvement. [Package] Data /packages/data labels May 5, 2019
@nerrad nerrad added this to the 5.7 (Gutenberg) milestone May 5, 2019
@nerrad nerrad self-assigned this May 5, 2019
- also fixes some wrapping in earlier changelog entry
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

I think based on the availability of useContext and its usage pattern, we should want to expose this object anywhere we also expose the provider and consumer subcomponents. If agreeable, we should consider auditing and creating a tracking issue.

For confirmation, this capitalization (UpperCamelCase) does seem to be what is promoted by React in their documentation:

https://reactjs.org/docs/hooks-reference.html#usecontext

@@ -381,6 +381,10 @@ _Returns_

Undocumented declaration.

<a name="RegistryContext" href="#RegistryContext">#</a> **RegistryContext**
Copy link
Member

Choose a reason for hiding this comment

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

Can we make sure this is documented? See also #15176, which proposes documentation for RegistryConsumer and RegistryProvider.

@nerrad
Copy link
Contributor Author

nerrad commented May 21, 2019

I forgot about this pull! I think we should actually either close this or rework it so that it instead just exposes the useRegistry hook instead of the entire context object (see https://github.com/WordPress/gutenberg/pull/15737/files#diff-f777130d04381cfeacdaeb4005e1f670) as a part of #15737.

@aduth
Copy link
Member

aduth commented May 21, 2019

@nerrad But useContext seems like something we should want to support for anywhere we're already providing a Consumer component, or do you disagree?

@nerrad
Copy link
Contributor Author

nerrad commented May 21, 2019

But useContext seems like something we should want to support for anywhere we're already providing a Consumer component, don't you think?

In the case of for the registry, I think any implementation would just use useRegistry instead of useContext. Effectively the only reason for useContext( RegistryContext ) would be to return the same as what useRegistry would.

We'd still need to export RegistryProvider and RegistryConsumer both for back-compat and because RegistryProvider will still need to wrap any component tree implementing either the consumer or the useRegistry hook.

@aduth
Copy link
Member

aduth commented May 21, 2019

Okay, this seems like a good conservative default for now. My thinking was less about registry context specifically, but context consumers generally. I think we tend to not expose the consumers anyways (intentionally?), so no need to be premature with exposing the context object I suppose. The only potential downside is that if we do choose to have a generic pattern, we'd need to decide how to reconcile this with useRegistry. Either a useFoo for every context available, or inconsistency of useRegistry vs. useContext( FooContext ), or some legacy we have to carry with us.

@nerrad
Copy link
Contributor Author

nerrad commented May 21, 2019

Either a useFoo for every context available, or inconsistency of useRegistry vs. useContext( FooContext ), or some legacy we have to carry with us.

Good points there, but I think context (a a general term, not an intentional pun!) matters here. I think we should generally avoid exposing low level things from wp.data unless demonstrated to be necessary and rather use available apis for exposing (which in this case would be useRegistry). I don't think we can come up with a blanket rule for all usages of Context across different packages.

@nerrad
Copy link
Contributor Author

nerrad commented May 21, 2019

Fwiw, we probably could avoid exposing useRegistry as a public api initially too assuming it's just implemented in useSelect or useDispatch?

@nerrad
Copy link
Contributor Author

nerrad commented May 27, 2019

This pull is now invalid. There is a useRegistry hook exposed in #15737 that has been merged.

@nerrad nerrad closed this May 27, 2019
@youknowriad youknowriad deleted the FET/export-entire-registry-context branch May 28, 2019 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Data /packages/data [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants