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

feat(@clayui/core): Add Collection implementation #5013

Merged
merged 15 commits into from
Aug 17, 2022

Conversation

matuzalemsteles
Copy link
Member

@matuzalemsteles matuzalemsteles commented Jul 26, 2022

Closes #5009
RFC 0004 Collection component

This PR mainly implements the virtualization pattern for the collection, there are some challenges that virtualization may not work very well in more complex cases in DropDown for example, also to create virtualization for Tree, we will have to deal with something like react-vtree does so I'll be working on it in another issue.

This is still a draft so other people can see and discuss something about if they find any improvements, we still need to add the cursor feature to useResource so that we can integrate asynchronous loading with the Collection.

ToDo

  • Asynchronous loading
  • Update the useResource hook documentation
  • Integrate the useResource hook with Suspense and ErrorBoundary
  • Add tests for useResource hook
  • Refactor cache implementation and unify implementation with promise control using <Provider /> in useResource hook

@matuzalemsteles matuzalemsteles force-pushed the collection branch 2 times, most recently from ca1d545 to d49f870 Compare July 29, 2022 00:10
@matuzalemsteles
Copy link
Member Author

I've pushed some more resources, adding support for asynchronous paginated data loading to the useResource hook, example usage:

const {loadMore, resource} = useResource({
  fetch: async (link: string) => {
    const result = await fetch(link);
    const json = await result.json();

    return {
      cursor: json.info.next,
      items: json.results,
    };
  },
  link: 'https://rickandmortyapi.com/api/character',
  variables: {limit: 10},
});

Extends the fetch implementation to be able to add the possibility to return a cursor instead of creating an API itself, this does not break the previous implementation of passing custom fetch or customizing, this is still fully compatible.

This covers a part of the RFC for asynchronous loading to list, this doesn't work well for tree because to deal with tree we need to do more complex logic, so it's quite likely that in another issue I'll be creating a hook that extends from useResource to be the useTreeResource.

@matuzalemsteles
Copy link
Member Author

matuzalemsteles commented Jul 29, 2022

I sent one more commit with an integration with <Suspense /> and <ErrorBoundary />, I still need to update the tests and add tests to cover this scenario.

There are still some things to be worked on in the integration, we need to refactor the cache implementation to better handle the data control, we need to make something like a client that can handle promises, cursors and the cache, since we are integrating with Suspense , when there is a promise in progress it loses data that is in some state or useRef. For this to work well we need to move this information out of the hook, we can use the Provider to initialize the "client" instance and retrieve it in the hook and also offer the option without, but we need to ensure that this data is outside the hook but we need to remove it when the component is actually dismantling, I believe this case is more difficult but I need to investigate further.

To use the integration is optional, this allows people to adopt it gradually otherwise we could cause a lot of breaking change, once it is enabled it is necessary for people to use it correctly with <Suspense /> and <ErrorBoundary />.

const {resource} = useResource({
  link: 'https://rickandmortyapi.com/api/character',
  suspense: true,
  variables: {limit: 10},
});

There is also a behavior that I added in the RFC that needs a review in the ErrorBoundary session, an idea I had in mind is that calling loadMore on the component where it will trigger the load could catch the error and throw a error so that the ErrorBoundary component catches this but this case doesn't work because ErrorBoundary only catches errors at render time and not in event handlers.

@matuzalemsteles
Copy link
Member Author

Well, I'm back again for this PR, sent a few more changes to refactor the cache implementation to work better with the Suspense and ErrorBoundary integration, it got simpler but we also need the <ClayProvider /> to be declared to start the client, too I added the possibility of not needing to declare if the provider is not declared to maintain compatibility but it is ideal for developers to declare it for when the application disassembles it can clean.

I made some minimal fixes, for example when the link property is a function, we use a uid to keep the data cached. There is an edge case for the Suspense integration to work well, when suspense is enabled we change the fetchPolicy to CacheAndNetwork or CacheFirst so we can retrieve the data.

I will do some more tests and add more tests to cover the new features of useResource and update the documentation.

@matuzalemsteles
Copy link
Member Author

I sent a few more commits that update the useResource hook documentation to talk about the new features of the hook and inform about some changes to existing properties and recommend not using it anymore.

I also made a strategy so that clayui.com can generate the Table API for the useResource hook, I had to make a fake component so that react-docgen can get the property information because it doesn't support hooks , this component is not exposed.

I also updated the style of the Table API in the documentation to improve the readability of the property descriptions and organized better how we were separating the information, it was getting more and more confusing to understand the table due to formatting and the little space.

Before After
Screen Shot 2022-08-05 at 6 37 51 PM Screen Shot 2022-08-05 at 6 34 55 PM

@matuzalemsteles
Copy link
Member Author

I sent a few more changes to add testing to the useResource hook. Well this PR is ready for a review now in case anyone wants to do it, it brings a lot of resources mainly for the useResource hook and the OOTB virtualization implementation for the Collection, for now only the VerticalBar component is using this component and has this functionality for be enabled as well as the Collection default that was adopted from the beginning.

To finalize the implementation of Collection, we need to add tree support, I created an issue to handle this #5030. I will also create an issue to review the RFC that comments on the integration with ErrorBoundary. I'm going to create another issue as umbrella to add the list of components that we will be adding to the Collection with pattern.

… ErrorBoundary

This commit adds the initial integration with React.Suspense for data fetching.

This is not a final integration because we need to refactor the cache implementation to be global and move the data from PROMISES and CURSORS to a global instance to simplify the implementation, we are using PROMISES as a data source to retrieve the values in some moments.

Since is integrated with Suspense and there is a promise in progress the component loses any state value or something that is with `useRef`, so we need to move the cache to work global.
…nse and ErrorBoundary and improves the integration with the cache system and avoids redundancy
…h Suspense and use Infinite loading with cursor usage in the documentation
…enerate the API table for the useResource hook
@matuzalemsteles
Copy link
Member Author

I'm going ahead and merging this PR!

@matuzalemsteles matuzalemsteles merged commit 4e222b3 into liferay:master Aug 17, 2022
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.

Implement the internal component of Collection
1 participant