Skip to content
This repository has been archived by the owner on Jul 12, 2024. It is now read-only.

WP Data should handle resolution status #4384

Closed
joshuatf opened this issue May 15, 2020 · 6 comments
Closed

WP Data should handle resolution status #4384

joshuatf opened this issue May 15, 2020 · 6 comments
Labels
status: stale The issue/PR is stale type: task The issue is an internally driven task (e.g. from another A8c team).

Comments

@joshuatf
Copy link
Contributor

joshuatf commented May 15, 2020

Now that things are moving towards wp data, we can remove specific selectors that check for isSomethingRequesting and use the built-in selectors in wp data.

For example, this call is no longer needed:

yield setIsRequesting( 'getJetpackConnectUrl', true );

And call to check for requesting can be replaced with:
isResolving()

https://github.com/WordPress/gutenberg/blob/1e8d78189a9b05628b171016e102e2c50e267c39/packages/data/src/namespace-store/metadata/selectors.js#L55

This blog post is also a very good overview of some of the available built-in selectors. Big thanks to @nerrad for these suggestions and taking a look at our current implementation.

@joshuatf joshuatf added the type: task The issue is an internally driven task (e.g. from another A8c team). label May 15, 2020
@nerrad
Copy link
Contributor

nerrad commented May 15, 2020

Also should be noted that the current implementation of isRequesting in the plugins resolver is prone to bugs because getJetpackConnectionUrl can be called with a query argument.

Resolver state is cached by selector name and arguments. So every time the query argument changes, the resolver will be called. When this happens you could potentially have multiple in flight requests but as soon as one of them is finished setIsRequesting( 'getJetpackConnectUrl', false ) is called and thus isRequesting will be false even if other in flight requests are not complete yet.

It's possible this didn't surface as a bug anywhere because of the way things are currently implemented, but the pattern certainly could produce bugs if used elsewhere.

All that to say, it's good you're doing a review of this pattern 👍

@github-actions
Copy link

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the status: stale The issue/PR is stale label Jan 26, 2021
@github-actions
Copy link

This issue was automatically closed due to being stale. Please feel free to re-open it if you still experience this problem.

@pmcpinto pmcpinto reopened this Feb 4, 2021
@github-actions
Copy link

github-actions bot commented Feb 9, 2021

This issue was automatically closed due to being stale. Please feel free to re-open it if you still experience this problem.

@github-actions github-actions bot closed this as completed Feb 9, 2021
@pmcpinto pmcpinto removed the status: stale The issue/PR is stale label Feb 9, 2021
@pmcpinto pmcpinto reopened this Feb 9, 2021
@github-actions
Copy link

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the status: stale The issue/PR is stale label Apr 10, 2021
@github-actions
Copy link

This issue was automatically closed due to being stale. Please feel free to re-open it if you still experience this problem.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: stale The issue/PR is stale type: task The issue is an internally driven task (e.g. from another A8c team).
Projects
None yet
Development

No branches or pull requests

3 participants