Add context argument to registry.select() by coderkevin · Pull Request #11460 · WordPress/gutenberg

coderkevin · 2018-11-03T18:15:32Z

Description

This adds an optional context argument to registry.select() and then adds support for withSelect to pass the component as a context for a select operation.

Why is this needed?

This makes it possible to map select calls to specific components, which means a data system can have knowledge of which components depend on which data. This gives better options for shared data handling that can be tied to the lifecycle of a given component.

How has this been tested?

All existing tests still pass, and additional tests have been added to ensure the operation of this new, optional feature.

Types of changes

Adds an optional argument to register.select( reducerKey, context).
Adds code to withSelect to supply a context of { component }.

Both changes are fully backwards-compatible.

Checklist:

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

tofumatt

Looks like this requires a rebase to fix the merge conflicts, but I'm also curious why this is needed more specifically? What issue will making the change solve? Is there a particular use case/example you could provide that would use this code? I think it would help to have some extra context here. 😄

coderkevin · 2018-11-14T23:20:33Z

Rebased on current master.

@tofumatt I'm trying to integrate another more advanced data system, fresh-data as a generic data store within @wordpress/data. That data system maps API data to the components which need it, which allows it to update its data graph as components mount and unmount, which is really helpful when calculating things like freshness requirements for automatic re-fetching, but this will also be used to clear out API data that was once fetched but no longer needed by any currently mounted component.

With this change, the differences between these systems become transparent to the application developer. Without this change, I'd be relegated to implementing my own withSelect HOC and thus not 100% compatible with other @wordpress/data store implementations.

tofumatt

Seems to work for me and I just have a few little comments, but I think this requires documentation updates before merging.

CC'ing @youknowriad and @aduth; not sure this is okay to merge at this point in the API freeze times.

packages/data/src/components/with-select/test/index.js

packages/data/src/test/registry.js

This adds an optional `context` as a second argument to `registry.select()` and also the generic store's `getSelectors()` function. This enables context-mapped selectors, which helps with declarative data requirements based on things like UI components when they become mounted and unmounted.

This adds a context of { component: this } to withSelect's call to registry.select. It does this by wrapping registry.select with an anonymous function that fills in the context when it's called.

coderkevin · 2018-11-21T00:01:36Z

Rebased onto WordPress/master

This adds documentation in the wp.data readme for the use of context. It also renames `genericStore` to `customStore` in the tests.

coderkevin · 2018-11-21T00:31:32Z

@tofumatt (and others), I've added docs and updated as requested.

This adds temporary code for a `withSelect` function outside of `@wordpress/data` until the context PR is merged: WordPress/gutenberg#11460

* data: Add @fresh-data/framework to package.json * data: Add fresh-data and replace orders in table This PR adds fresh-data with a WooCommerce API spec to fulfill order information. It then replaces the existing selectors for the orders table with the new selectors as a proof-of-concept. * wc-api: Add temporary code for `withSelect` This adds temporary code for a `withSelect` function outside of `@wordpress/data` until the context PR is merged: WordPress/gutenberg#11460 * wc-api: Update fresh-data to 0.5.0

tofumatt

Really sorry this took ages to get to—it got buried in my review queue. But the changes look good; if you can rebase onto master then I think this is good to go?

aduth · 2019-01-04T15:29:33Z

packages/data/README.md

 registry.registerGenericStore( 'custom-data', createCustomStore() );
 ```

+Note in the above example the handling of the optional `context` argument to `getSelectors()`. When using `withSelect`, this provides information about which component is requesting which data from the store. The benefit of this is the ability to keep track of which data is still currently needed by currently mounted components and which data is no longer needed.


The benefit of this is the ability to keep track of [...] which data is no longer needed

How would this be achieved? In the above example, the cache of componentResources will grow infinitely (which in its own right is very problematic). A component will not be removed if it's unmounted, nor is it obvious how this would be accomplished.

The implementation I have for wc-admin removes components from the map upon unmount. The only components in the map are ones currently mounted at any given point in time.

coderkevin · 2019-01-22T16:39:35Z

Hey there, I just wanted to let you know that I've got this working with a separate with-select component in our repo. I'll reply again here later to discuss more details on how to make this work in a more general way as intended originally.

gziolo · 2019-02-04T13:09:26Z

@coderkevin does it mean this PR can be closed or do you plan to refresh it based on the revised solution you figured out?

By the way, we are setting up some more detailed structure for reviewing PRs. Would you like to get automated pings for review for the code inside packages/data folder given your very deep understanding of this area? Related PR: #13604 and parent issue #13441.

coderkevin · 2019-02-04T16:20:08Z

does it mean this PR can be closed or do you plan to refresh it based on the revised solution you figured out?

I'm not sure yet. I haven't yet circled back to this PR but hope to in the next week.

Would you like to get automated pings for review for the code inside packages/data folder given your very deep understanding of this area?

Sure, I'd be happy to help review anything in packages/data. Thanks!

See related discussion #11460 (comment)

@swissspidy

* Workflow: Add repository CODEOWNERS file * Fix duplicate names * Project: Add @swissspidy as i18n package code owner * Remove duplicated /packages/rich-text * Plugin: List individual code owners instead of core group * Adding @coderkevin See related discussion #11460 (comment) * Plugin: Add mobile (native.js) maintainers as code owners

@swissspidy

* Workflow: Add repository CODEOWNERS file * Fix duplicate names * Project: Add @swissspidy as i18n package code owner * Remove duplicated /packages/rich-text * Plugin: List individual code owners instead of core group * Adding @coderkevin See related discussion #11460 (comment) * Plugin: Add mobile (native.js) maintainers as code owners

@swissspidy

* Workflow: Add repository CODEOWNERS file * Fix duplicate names * Project: Add @swissspidy as i18n package code owner * Remove duplicated /packages/rich-text * Plugin: List individual code owners instead of core group * Adding @coderkevin See related discussion #11460 (comment) * Plugin: Add mobile (native.js) maintainers as code owners

timmyc · 2019-06-06T15:31:20Z

@coderkevin are we still going to try and merge this in at some point?

gziolo · 2019-06-07T08:04:41Z

This PR would have to cover also newly introduced useSelect hook. I'm marking this PR as stale to reflect that it's fairly outdated.

coderkevin · 2019-06-07T14:04:04Z

@timmyc Thanks for the reminder. I've been working on a modification to fresh-data that would no longer require the special handling needed in this PR. So hopefully this PR won't be needed after the next major version of fresh-data.

nerrad · 2019-08-13T22:43:34Z

Thanks for the reminder. I've been working on a modification to fresh-data that would no longer require the special handling needed in this PR. So hopefully this PR won't be needed after the next major version of fresh-data.

How is this coming @coderkevin? Will this PR (or some variation of it, considering the staleness) be needed?

nerrad · 2019-12-04T13:29:40Z

Things have changed in the project inspiring this original pull, so it can be closed.

tofumatt reviewed Nov 14, 2018

View reviewed changes

coderkevin mentioned this pull request Nov 15, 2018

Add wc-api and "Fresh Data" woocommerce/woocommerce-admin#833

Merged

tofumatt requested changes Nov 16, 2018

View reviewed changes

packages/data/src/components/with-select/test/index.js Outdated Show resolved Hide resolved

packages/data/src/components/with-select/test/index.js Outdated Show resolved Hide resolved

packages/data/src/test/registry.js Outdated Show resolved Hide resolved

tofumatt requested review from aduth and youknowriad November 16, 2018 18:47

coderkevin added 2 commits November 20, 2018 18:00

data: Add context to select within withSelect

3f0fc8c

This adds a context of { component: this } to withSelect's call to registry.select. It does this by wrapping registry.select with an anonymous function that fills in the context when it's called.

data: Add doc for getSelectors( context )

f77e80c

This adds documentation in the wp.data readme for the use of context. It also renames `genericStore` to `customStore` in the tests.

tofumatt reviewed Jan 4, 2019

View reviewed changes

aduth requested changes Jan 4, 2019

View reviewed changes

gziolo added [Package] Data /packages/data [Type] Enhancement A suggestion for improvement. Needs Decision Needs a decision to be actionable or relevant labels Feb 4, 2019

gziolo added a commit that referenced this pull request Feb 4, 2019

Adding @coderkevin

a8dac98

See related discussion #11460 (comment)

gziolo added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Jun 7, 2019

nerrad closed this Dec 4, 2019

yassine4999 mentioned this pull request Nov 30, 2023

[Snyk] Fix for 1 vulnerabilities yassine4999/gutenberg#29

Open

Conversation

coderkevin commented Nov 3, 2018

Description

Why is this needed?

How has this been tested?

Types of changes

Checklist:

Uh oh!

tofumatt left a comment

Choose a reason for hiding this comment

Uh oh!

coderkevin commented Nov 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tofumatt left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderkevin commented Nov 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderkevin commented Nov 21, 2018

Uh oh!

tofumatt left a comment

Choose a reason for hiding this comment

Uh oh!

aduth Jan 4, 2019

Choose a reason for hiding this comment

Uh oh!

coderkevin Jan 22, 2019

Choose a reason for hiding this comment

Uh oh!

coderkevin commented Jan 22, 2019

Uh oh!

gziolo commented Feb 4, 2019

Uh oh!

coderkevin commented Feb 4, 2019

Uh oh!

timmyc commented Jun 6, 2019

Uh oh!

gziolo commented Jun 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderkevin commented Jun 7, 2019

Uh oh!

nerrad commented Aug 13, 2019

Uh oh!

nerrad commented Dec 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

coderkevin commented Nov 14, 2018 •

edited

Loading

tofumatt left a comment •

edited

Loading

coderkevin commented Nov 21, 2018 •

edited

Loading

gziolo commented Jun 7, 2019 •

edited

Loading