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: Expose hasResolver property on returned selectors #15436

Merged
merged 2 commits into from
May 16, 2019

Conversation

nerrad
Copy link
Contributor

@nerrad nerrad commented May 5, 2019

Description

While implementing the new RESOLVE_SELECT control as a part of #13716, I remarked in a comment that it would be more dev friendly if the select control would automatically handle when there's a resolver involved. In order to do that, wp.data needs to expose whether a selector has a resolver. I figured the cleanest way to do this would be to add a hasResolver property to the selectors while they are mapped to resolvers.

How has this been tested?

Types of changes

This is an enhancement and a non-breaking change.

Other:

I didn't add much documentation about this new property as I think its primarily for low-level usage. Should there be more documentation? I think the code (and tests) is fairly clear on its intended purpose?

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 nerrad added [Type] Enhancement A suggestion for improvement. [Package] Data /packages/data labels May 5, 2019
@nerrad nerrad requested review from aduth and youknowriad as code owners May 5, 2019 02:25
@nerrad nerrad self-assigned this May 5, 2019
@nerrad nerrad added this to the 5.7 (Gutenberg) milestone May 5, 2019
@youknowriad youknowriad removed this from the 5.7 (Gutenberg) milestone May 14, 2019
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

The implementation seems fine, the problem I have with this though, is that "resolvers" is something specific to the namespace store implementation.

This means a potential generic SELECT control will have to know about the existence of a custom store implementation supporting resolvers. Is it a fine tradeoff, I don't know? Do you think we could find a way to build this control by being store-implementation agnostic?

@nerrad nerrad force-pushed the FET/expose-when-selector-has-resolver branch from 64e2118 to c0f6553 Compare May 16, 2019 11:10
@nerrad
Copy link
Contributor Author

nerrad commented May 16, 2019

Great catch @youknowriad, I added a test that demonstrates the problem. I like the implementation of exposing a property on the selector so the fix involves just initializing the property in the selector mapper (see c0f6553)

@nerrad nerrad requested a review from youknowriad May 16, 2019 11:12
@youknowriad
Copy link
Contributor

The issue is not really fixed for me. Let me explain better.

Think of the "namespace" stores as a custom implementation of stores, it could even be moved outside the data module. The data module by default support generic stores { subscribe, getActions, getSelectors }. In this generic interface, there's no concept of "resolver", it's something specific to the namespace stores.

This brings me to this line https://github.com/WordPress/gutenberg/pull/15435/files#diff-9fb42816127c9fa3299026bdc8a283dfR160

Why a generic data-controls package, containing a generic SELECT control should know about of the existence of a custom store implementation (that make use of resolvers) and use the hasResolver flag?

@nerrad
Copy link
Contributor Author

nerrad commented May 16, 2019

Ahh I think I understand better now.

Why a generic data-controls package, containing a generic SELECT control should know about of the existence of a custom store implementation (that make use of resolvers) and use the hasResolver flag?

Specially in the case you linked to, it's so that there's no need for a resolveSelect control since calling the select control will automatically handle doing a resolveSelect if the selector has a resolver. However, in light of your comments, an extrapolation of your comments then is the data-controls store should not expose a resolveSelect control because that's an implementation detail of a custom store.

So is the way forward here to close this pull and just have the new data-controls package expose only generic controls?

@youknowriad
Copy link
Contributor

youknowriad commented May 16, 2019

However, in light of your comments, an extrapolation of your comments then is the data-controls store should not expose a resolveSelect control because that's an implementation detail of a custom store.

So is the way forward here to close this pull and just have the new data-controls package expose only generic controls?

That's where I'm hesitant. I think there's definitely value in that control and it should be something used across modules (select the "final" result of a selector) that's why I was asking if it's a fine tradeoff.

That said, ideally and I don't know if it's possible, can we find a way to make that control work without having to "leak" a store implementation detail? This probably means introducing some notion of completeness to selectors across store implementations, not sure if it's worth the effort. cc @aduth

@nerrad
Copy link
Contributor Author

nerrad commented May 16, 2019

Part of the issue here I think though is that "controls" itself is an implementation of namespaced stores right (i.e. it's not something proffered by generic stores unless they choose to implement). So by that definition, the controls package only has value to those implementations that are using resolvers (or at least using a namespaced store)?

@youknowriad
Copy link
Contributor

@nerrad you're totally right, how did I miss that.

I guess it means it's not really an issue.

@nerrad nerrad merged commit 1f52970 into master May 16, 2019
@youknowriad youknowriad deleted the FET/expose-when-selector-has-resolver branch May 16, 2019 14:06
@nerrad nerrad added this to the 5.8 (Gutenberg) milestone May 16, 2019
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.

2 participants