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

[WIP] Try/fresh data as data plugin #8754

Closed
wants to merge 3 commits into from
Closed

[WIP] Try/fresh data as data plugin #8754

wants to merge 3 commits into from

Conversation

coderkevin
Copy link
Contributor

@coderkevin coderkevin commented Aug 9, 2018

Note: This PR is not yet fully baked and ready to merge. It still needs further discussion.

Description

This adds fresh-data functionality to @wordpress/data through the plugin use mechanism to adjust the registry in the following ways:

  • Allows registerStore( reducerKey, options ) to be called with apiSpec as an option, which corresponds to the apiSpec object in fresh-data.
  • Adds a getApiClient( reducerKey ) function to the registry.

This PR also adds a withResources HOC which is similar to the withSelect HOC. It's different because it needs a mapping to the component which is requiring the resource, and I really want to set the requirements of a component in one single operation, rather than clearing the requirements then re-adding them, which can get messy.

The reasoning behind this addition is to support more advanced asynchronous API data needs like for the WooCommerce API and other applications which are data-heavy.

This adds the following functionality that does not presently exist in @wordpress/data:

  • Freshness requirements set within the Component HOC.
  • No need to fetch data that has been fetched recently by another component (or block).
  • Coordinating for the same resource which is needed by multiple components.
  • Pagination fetching which is still compatible with individual items of that page.
  • Updating of internal state from return values of mutating API calls.
  • Fetching multiple resources using a single API call (e.g. using includes param with ids).
  • Fetching multiple resources in one API call even if they're required by different components.

This mechanism still also supports the same features we know and love with @wordpress/data such as:

  • Declarative component interface via the HOC.
  • Automatic resolving of data and re-rendering after data arrives.
  • Registration of multiple flavors of data.
  • Custom selectors/mutations.

Implementation details:

This implementation lays out a mapping of connected components and their data requirements. It then reduces those requirements into an actionable list of resources to be fetched. The mapping of components is critical here, because it allows the resource requirements list to be updated in accordance to component lifecycle. For instance, when a component mounts, it requires the data it needs as referenced by its props (e.g. post #25). When it unmounts, those requirements are cleared from the system and no further fetching of that data will occur (unless there's another component requiring it too, of course). It's by this mechanism that duplicate fetches are avoided without resorting to debouncing. Only one part of the code ever fetches data, and it does it in accordance with the complete mapping of all data required for that api.

Further notes:

Through this process, I've discovered some places where fresh-data can be simplified without reducing functionality. These changes will also provide closer integration points to the @wordpress/data code. I am working on these changes and intend to update this code to work with them after they are all merged.

Integration:

This functionality is necessary for more complex API applications. However, I'm open to discussion about how best to integrate these changes. Let's have a discussion about it.

Also, with the addition of the plugin mechanism, it should be possible for this code to exist entirely outside of the @wordpress/data package if so desired. (with one caveat: RegistryConsumer would need to be exported from @wordpress/data for the HOC to be possible.)

How has this been tested?

I created a PR in wc-admin which uses @wordpress/data as a dependency and implements some WooCommerce API calls: woocommerce/woocommerce-admin#248

See the instructions on that PR for testing.

Types of changes

  • Adds plugins/fresh-data to @wordpress/data exports.
  • When used, the fresh-data plugin adds apiSpec and getApiClient to registry.
  • Adds withResources HOC.

No breaking changes.

Checklist:

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

@coderkevin coderkevin changed the title Try/fresh data as data plugin [WIP] Try/fresh data as data plugin Aug 9, 2018
This adds fresh-data functionality to @wordpress/data through a
`registerApi` function (in lieu of `registerStore`,
`registerActions`, etc.) And using a new `withResources` HOC.
This adds mutations to the `registerApi` options, and also a
`mapMutationsToProps` function to the `withResources` HOC.
Use the new plugin mechanism within `@wordpress/data` to try out
fresh-data to support advanced data features.
@coderkevin coderkevin mentioned this pull request Aug 21, 2018
6 tasks
@aduth
Copy link
Member

aduth commented Aug 21, 2018

While this seems like a good use-case for a data plugin (and a stress test of its capabilities), could you explain for what reason this should be considered for inclusion in the core module itself, vs. distributed separately? How is this planned to be used in a core development context? How does it relate to similar efforts like queried-data (#8357, specifically queries & pagination)? Could it interoperate with resolution state to incur freshness-based invalidation there?

As it stands, it seems to be a very isolated approach and it's not clear if / how this would be introduced into core development patterns. If it's not intended for use in core features, this is precisely the sort of thing which could make sense as an externally-referenced plugin (e.g. wp.data.use( require( 'my-data-plugin' ) );).

@coderkevin
Copy link
Contributor Author

coderkevin commented Aug 23, 2018

for what reason this should be considered for inclusion in the core module itself, vs. distributed separately?

Honestly, other than the benefits of a data system like this, I don't have any direct reasons.

How is this planned to be used in a core development context?

If by "core" you mean WordPress core and Gutenberg, I'm not qualified to answer that as I haven't been part of the core development team for Gutenberg. However, for larger, data-dependent applications like WooCommerce, there is definite desire to use this for the new dash work and beyond.

How does it relate to similar efforts like queried-data?

There is overlap, but fresh-data incorporates more features than queries and pagination, of course. More importantly, I think, is the way fresh-data organizes data by resource name, which lends itself to mapping queries to existing data in the cache by resource name. In short, API data can be stored in one place (by ID, for example), and referenced by multiple search or page queries.

Could it interoperate with resolution state to incur freshness-based invalidation there?

This might be tricky. I've found in order to support a concept of freshness well, the freshness requirements need to be driven by the application code (read: application components). This means more than one component may require a resource, but with different freshness requirements (e.g. A large list may only need them fresh to within 1 hour, while an edit screen may need it much more fresh.) This isn't really possible without mapping resource requirements to components, where they can be reduced to a single set of requirements and be updated every time a component mounts or unmounts. And unfortunately, no component context is passed via withSelect and with the way selectors are mapped and passed, they don't support being pre-curried with the component context before being passed to mapStateToProps from within withSelect.

In summary: If a demand-based freshness concept is to be introduced in wp.data, it will need to be modified to support component mapping to resources (a la withResources in this PR).

As it stands, it seems to be a very isolated approach and it's not clear if / how this would be introduced into core development patterns.

While I really don't agree with the "isolated approach" comment, I do agree that there is a fundamental difference between this approach and what is currently in wp.data. However, it's for a reason, and if wp.data were to ever support what fresh-data does, it would need to do so in a similar way.

I guess the question is: Will wp.data will ever need to work with data in this way? I'm afraid I can't answer that, as I only know my own needs.

Personally, I'm comfortable using this as an external plugin as long as we have the relevant teams' blessings to use this in a project like wc-admin in that way. If I were making the decision on this today, I'd say let's use it as an external plugin now, as referenced here: woocommerce/woocommerce-admin#301 And we can always revisit this in the future if wp.data needs similar functionality.

@aduth
Copy link
Member

aduth commented Aug 23, 2018

Aside: I see that the comment has since been edited, but I sense that there was some offense taken at the use of the phrase "isolated approach". My intention was to summarize that it should be a requirement of this proposal to consider its interoperability with existing tooling (for which there are overlapping goals) or a proposal for how we'd consider to migrate those toward this alternative solution. A goal of @wordpress/data is to provide a cohesive story of solutions. If you have suggestions for how I could have communicated this better, I'd appreciate the feedback on my review process. It had me considering whether it was a piece of feedback I should have regretted mentioning, though in hindsight I think it was an important point of summary.

@coderkevin
Copy link
Contributor Author

Re: the "isolated approach" comment, I think it's pretty easy to take that language as a dismissive judgement. (i.e. not a team player, etc.) but I know you didn't mean it that way. I didn't want to communicate it as such, so I edited my response. "Isolated" is much stronger language than "outside of the wp.data development effort" which is what you seemed to mean from your clarification.

...it should be a requirement of this proposal to consider its interoperability with existing tooling...

This was the primary intent of this PR, in fact. It's a first step in interoperability between the two data systems.

...a proposal for how we'd consider to migrate those toward this alternative solution.

I figured it might be a bit premature to put a lot of work toward something that may not even be desired. I put forth this PR to gauge interest in including it in @wordpress/data itself first. Why waste time working on something that the maintainers don't want anyway?

I'm happy to work on reconciling these two different approaches if it's something that the maintainers of this project would want. I'm only slightly less happy to keep it as an external plugin, so long as my teammates can use it for their work. 😄

@coderkevin
Copy link
Contributor Author

This PR is no longer needed with the registerGenericStore support in registry..

@coderkevin coderkevin closed this Nov 5, 2018
@coderkevin coderkevin deleted the try/fresh-data-as-data-plugin branch November 5, 2018 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants