feat(@clayui/data-provider): dedupe concurrent requests

[matuzalemsteles]

This dedup of concurrent requests to the same address, this is for cases when the same component is displayed twice on the screen, a strategy that the application does to mitigate this is to move to above of component with useResource state, but now it is not more necessary because we have normalized everything, it makes the experience more comfortable for the developer.

I think this is part of other details that I want to implement in the data provider to encourage good standards in dealing with the flow of data in the applications, using suspense, fetching early... I will work more on this after I am finished with the migration DDM-field-type... probably 😁!

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 2f15543:

Sandbox	Source
snowy-hooks-zmtr9	Configuration
practical-waterfall-4fcxb	Configuration

[coveralls]

Pull Request Test Coverage Report for Build 4737

• 8 of 10 (80.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.02%) to 78.515%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/clay-data-provider/src/useResource.ts	8	10	80.0%

Totals
Change from base Build 4734:	-0.02%
Covered Lines:	2303
Relevant Lines:	2734

---

💛 - Coveralls

[bryceosterhaus]

This seems odd to me to have two = within the same expressing. Is this intended?

[bryceosterhaus]

If it is intended, I think its probably best to separate them into two separate expressions since it'd be easy to miss while reading through.

[matuzalemsteles]

I added this because we are unable to support when the link API is a function, we are unable to know what the endpoint is. It just adds a reference point for both, do you think it's a problem?

[bryceosterhaus]

I was thinking we should at least separate it into two separate declarations. Would this work?

promise = fetch( // ...

CONCURRENT_PROMISES[url] = promise

[matuzalemsteles]

Yeah, no problem. I will do this.

[bryceosterhaus]

How come you added this here?

[matuzalemsteles]

Ah yeah, that was because it was causing a 302 (Temporary redirect to the address with /), I added it to not see it anymore on the tab network 😁.

[wincent]

I think we can assume and use finally() instead of repeating the delete in both then() and catch() — all modern browsers support finally(), and core-js (which we use as a polyfill for IE in portal) has supported it for a while.

(I think that, as a library, Clay should be following a bring-your-own-polyfill approach like I said here).

[matuzalemsteles]

Oh yeah, I had forgotten about that method available on Promise, thanks!

[wincent]

I do feel a little queasy about this deduping happening auto-magically. What if I have an end point called /sample-colors that returns me a list of random colors every time I hit it? With change, if I had two components listing those colors on a single page, the behavior is now different (because I'll get the same colors in both components).

That's a contrived example, I know, but this change takes some control out of my hands about when/how things get fetched. (I could work around it in this example, by adding some random parameter on the end of the requests.) Do we have a real-world example of where the current behavior has caused problems? That would be helpful context to have.

[matuzalemsteles]

I do feel a little queasy about this deduping happening auto-magically. What if I have an end point called /sample-colors that returns me a list of random colors every time I hit it? With change, if I had two components listing those colors on a single page, the behavior is now different (because I'll get the same colors in both components).

Yeah 🤔, that's a good point! I think we can provide a property in useResource that makes it possible to disable this automatic mechanism, what do you think?

That's a contrived example, I know, but this change takes some control out of my hands about when/how things get fetched. (I could work around it in this example, by adding some random parameter on the end of the requests.) Do we have a real-world example of where the current behavior has caused problems? That would be helpful context to have.

I don't have a real use case yet just hypotheses but this is what I see happening in popular libs where I am researching, some examples:

• React Query (They use names to identify when to dedupe)
• SWR (Has the property to disable dedupe mode)
• React Apollo (This is a different case too it uses the name of the queries to be able to deduce making the dedupe and if I'm not mistaken it creates the key based on the query)

[wincent]

I don't have a real use case yet just hypotheses

For me that's a big sign that we shouldn't rush here (and also it's not clear to me why we're working on it):

• If we don't have the feature, no problem, because we haven't found a motivating use case for it yet.
• If we do have the feature, but it ends up being "wrong", we've might have increased our API surface area and overall complexity, and we now have an API that we're now on the hook for maintaining, documenting, and not making compatibility-breaking changes to.

The thing about useResource is that it is so generic there isn't actually a lot that we can assume about how/where it will be used (unlike you example of, say, React Apollo, which at least knows it is operating within the semantics of GraphQL), which makes this kind of thing much harder.

[matuzalemsteles]

Hey @wincent yeah, React Apollo has a lot more power to make those decisions I assume React Query and SWR is in the same direction.

My idea is not to increase the surface, of course we can take the risk here but rather increase the value in useResource so that it can handle more cases and enable new ways to use it. My idea is that people try to use useResource more as a standard and that we need it to add more value to them and solve problems.

I think we can wait for that as we don't have a case inside Liferay it's still likely that people won't notice this detail and maybe it's already happening 😁

[bryceosterhaus]

@wincent
If we do have the feature, but it ends up being "wrong", we've might have increased our API surface area and overall complexity, and we now have an API that we're now on the hook for maintaining, documenting, and not making compatibility-breaking changes to.

Yeah, this also makes me re-think the need to add this quite yet. Perhaps we can leave this as "on hold" for now and address in the future as we see a need for it.

Sidenote...
This PR also raised a few thoughts on the necessity of the package vs other 3rd party ones. I'm curious who is currently using this package and what sort of needs we are fulfilling with this package. If usage isn't very high at the moment, it makes me wonder if we might better off recommending a 3rd party library like the ones you mentioned since to classify this under "Clay" also seems a little odd since it has nothing to do with implementing lexicons design.

[matuzalemsteles]

Yeah, this also makes me re-think the need to add this quite yet. Perhaps we can leave this as "on hold" for now and address in the future as we see a need for it.

No problem for me!

This PR also raised a few thoughts on the necessity of the package vs other 3rd party ones. I'm curious who is currently using this package and what sort of needs we are fulfilling with this package. If usage isn't very high at the moment, it makes me wonder if we might better off recommending a 3rd party library like the ones you mentioned since to classify this under "Clay" also seems a little odd since it has nothing to do with implementing lexicons design.

I would say the same if I were to think about this line of reasoning but the data provider is a complement that adds value to Clay, many of our components need data and we can design an API oriented to our cases, the other tools try to do a lot, dealing with HTTP, GraphQL... not that it's bad but I think we can do something very exciting here with the thoughts that I put in issues.

It is probably something that we will work more on in our next Data to Table/List components where the idea is to facilitate the creation of these components in more high-level components.

Uma coisa legal que gosto do DataProvider, ele é leve, configurações minimas, muito poderoso para reutilizar pela aplicação com os mecanismos de cache, retry... and I find it very simple to start and grow.

[matuzalemsteles]

I would love this now haha, I avoided commenting on it here for a few weeks... I'm working on the migration from Form to React and I had noticed a use case for that: we there loads several components to create the form, we need a data that searches the server for a list of all available fields that we can use because clients can also create their own fields. We have a component that is responsible for rendering the layout of the fields, which is used to build a form and is also used to edit the fields themselves, used in the Sidebar.

The case is that this data is used only in the Field component, which is responsible for loading the fields on demand, as the migration is being little by little from the bottom up, there was no way to put the useResource at the root of the application to avoid problems with concurrents requests, so I raised the level for the PageRenderer which is capable of rendering a Forms page with several fields, this works but the PageRenderer set is rendered elsewhere which is in the Sidebar, it also uses the fields to edit their own fields, at times I can see more than one request. Finally, I avoided this because it could be bypassed in the next migration interaction which is the FormRenderer component, above, it is capable of rendering several pages and controlling some states, I once again raised the useResource to avoid requests when there is more than a page, this helped to lessen the problem but as I said above, we can have more than one FormRenderer rendered on the page, in some cases, I can see more than one request.

I can avoid this today, using cache policies and global storage to avoid another request when opening the Sidebar, but I ended up finding another problem with concurrent requests, we have a component that serves as compatibility to allow calling a specific component written in React in a part of the application that is still in Metal+Soy, with no provision to migrate that part yet... so anyway I go back to the problem, I have to add a useResource in the compatibility component that loads the field, but can there are many fields and this creates a lot of concurrent requests... In the end, this is a specific case but despite being able to get around this, I didn't want to climb the useResource to avoid these problems, I want to use it in the component where it is needed of the data, I want to use the useResource only when it's really necessary, this part of the application I'm trying to avoid using a global store because it's a component reusable... I know there is the case that Greg reported, so it would be great to be able to have a flag to enable this when necessary.

An option here also: initializes the application already with this data, but we do not want to touch the applications they are using, because it is used in other modules and we will need to reuse the servlet that does this... But I'll leave that decision to the team 😁, They still need to "put their own house in order"!

[wincent]

I think it's great that you have needs emerging out of first-hand experience trying to get things done @matuzalemsteles. That's going to lead us to a better design for sure.

[matuzalemsteles]

I think it's great that you have needs emerging out of first-hand experience trying to get things done @matuzalemsteles. That's going to lead us to a better design for sure.

Yeah, it sure has great value.

[bryceosterhaus]

@matuzalemsteles since this update will likely be moved over to the dxp specific implementation of data-provider, I am going to close this PR. If if needs to be reopened in the future, just let me know!