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: add usePager hook #798

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

gronxb
Copy link

@gronxb gronxb commented Dec 11, 2023

Summary

Related Issue #92

During my experience with react-native-pager-view, I encountered a limitation where accessing the pager view's features was predominantly dependent on using a ref. This reliance often led to less flexible and more complex implementations. Therefore, I implemented a hook for the methods.

However, I realized that simply exposing the existing methods - setPage, setPageWithoutAnimation, and setScrollEnabled - was insufficient, as there was no way to retrieve the current state information. Merely exporting these methods limited the scope of possible scenarios. Therefore, I found it necessary to include not only these methods but also values representing the current page, and flags indicating whether it's possible to move to the next or previous page.

The implementation was carried out using React Context, and components within the component can now utilize these features through the usePagerView hook. This approach ensures that all child components of have easy access to the pager view's capabilities.

  • Usage
const Item = () => {
  const {
    page,
    hasNextPage,
    hasPreviousPage,
    setPage,
    setPageWithoutAnimation,
    setScrollEnabled,
  } = usePagerView();

  return (
    <View style={styles.content}>
      <Text>Current Page: {page}</Text>
      <Text>hasNextPage: {String(hasNextPage)}</Text>
      <Text>hasPreviousPage: {String(hasPreviousPage)}</Text>
      <Button
        title="next page"
        onPress={() => {
          if (hasNextPage) {
            setPage(page + 1);
          }
        }}
      />
      <Button
        title="prev page"
        onPress={() => {
          if (hasPreviousPage) {
            setPage(page - 1);
          }
        }}
      />

      <Button
        title="next page without animation"
        onPress={() => {
          setPageWithoutAnimation(page + 1);
        }}
      />

      <Button
        title="setScrollEnabled to true"
        onPress={() => {
          setScrollEnabled(true);
        }}
      />

      <Button
        title="setScrollEnabled to false"
        onPress={() => {
          setScrollEnabled(false);
        }}
      />
    </View>
  );
};
Simulator.Screen.Recording.-.iPhone.15.-.2023-12-11.at.16.36.59.mp4

Test Plan

스크린샷 2023-12-11 오후 4 34 16 If usePagerView is not used within a component, it will throw an error for the developer's awareness.

What's required for testing (prerequisites)?

Inside the component, use the usePagerView hook

What are the steps to reproduce (after prerequisites)?

See Usage above

Compatibility

OS Implemented
iOS
Android

Checklist

  • I have tested this on a device and a simulator
  • I added the documentation in README.md
  • I updated the typed files (TS and Flow)

@troZee
Copy link
Member

troZee commented Dec 11, 2023

Hello,

I appreciate your contribution to this project. You have done a great job with your solution. 👏

I’m curious to learn more about your approach and how it differs from this approach from the example folder. Could you please explain to me what is the difference between them? I think we can both benefit from sharing our perspectives and insights. 😊

Thank you for your time and cooperation.

@gronxb
Copy link
Author

gronxb commented Dec 12, 2023

@troZee

Thank you for your reply.

First, useNavigationPanel seems to me like a hook designed to pass refs and props to PagerView. The hook I've created utilizes the React Context, allowing PagerView to act as a Provider, and targets the use of PagerView methods in its children. Although useNavigationPanel exports methods, it must be possible to pass them to children as props.

In the production environment I am working on, children of PagerView often experience props drilling. Therefore, I wanted to support a method that could be controlled within the <PagerView /> itself.

Issue #92 was posted in the past, and from the reactions to that issue, I realized that there were quite a few people who felt the same way I did.

Although it's a different case, react-navigation also allows child components to manage page navigation scenarios through useNavigation(). I approached PagerView with the same perspective.

  • Usage usePagerView hook
const App = () => {
    return (
        <PagerView>
            <Item />
        </PagerView>
    )
}

// child

const Item = () => {
  const {
    page,
    hasNextPage,
    hasPreviousPage,
    setPage,
    setPageWithoutAnimation,
    setScrollEnabled,
  } = usePagerView();

  return (
    <View style={styles.content}>
      <Text>Current Page: {page}</Text>
      <Text>hasNextPage: {String(hasNextPage)}</Text>
      <Text>hasPreviousPage: {String(hasPreviousPage)}</Text>
      <Button
        title="next page"
        onPress={() => {
          if (hasNextPage) {
            setPage(page + 1);
          }
        }}
      />
      <Button
        title="prev page"
        onPress={() => {
          if (hasPreviousPage) {
            setPage(page - 1);
          }
        }}
      />

      <Button
        title="next page without animation"
        onPress={() => {
          setPageWithoutAnimation(page + 1);
        }}
      />

      <Button
        title="setScrollEnabled to true"
        onPress={() => {
          setScrollEnabled(true);
        }}
      />

      <Button
        title="setScrollEnabled to false"
        onPress={() => {
          setScrollEnabled(false);
        }}
      />
    </View>
  );
};

{...this.props}
ref={(ref) => {
this.pagerView = ref;
<PagerViewContext.Provider
Copy link
Member

Choose a reason for hiding this comment

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

[high]:

React Context is a feature of React that allows you to pass data through the component tree without having to pass props down manually at every level. However, it is not a state management tool, and it has some performance implications that you should be aware of.

One of the main performance issues with React Context is that when a context value changes, all components that use the useContext hook or the Consumer component will re-render, regardless of whether they use the changed value or not. This can cause unnecessary re-rendering of components that are not affected by the context change, and thus affect the performance of your app.

[suggestion]

Would you mind creating a separate file called PagerViewContext and moving the Context there? I don’t want to affect other projects. Moreover, this is breaking changes due to behavior changes. 

After that, could you update the readme file with a section about the usePager hook?

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your suggestion

When separating the React Context Provider into a separate file, there are issues with inserting the value. The fundamental problem is that re-rendering occurs even when the usePager hook is not used. (Originally, it was named usePagerView, but I changed it following your suggestion.)

I thought it would be good to solve the re-rendering issue without changing the way it is used. Therefore, I chose to use useSyncExternalStore to configure the store and then inject it into the context, changing the value of the store.

Since only the store’s value changes, rendering does not occur in components that do not use usePager.

However, one concern is that useSyncExternalStore is a React 18 feature, which is not compatible with earlier versions. But since the example also uses version 18, and it has been a while since version 18 was released, I believe it would be fine if we insert a warning message.

Lastly, one issue I have been contemplating is that even with the current usage, re-rendering occurs when there is a change in the page, even if one only wants to use setPage. For example, const {setPage} = usePager();

In usePager(), we could export only methods like setPage, setPageWithoutAnimation, setScrollEnabled, and construct a selector like usePagerState((state) => state.page);. This would perfectly optimize rendering only for the parts subscribing to the state, but it might make the usage somewhat more cumbersome.

If you agree with the approach I am taking, I would appreciate your opinion on this matter.

Copy link
Author

@gronxb gronxb Dec 19, 2023

Choose a reason for hiding this comment

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

The <NonHookComponent /> inside the <PagerView /> does not use the usePager hook. (You can also see the code at the bottom of the video.) Additionally, I have set it up to output a console.log statement every time it renders.

The full code for this example can be found in the example code.

  • as-is render (only context)
as-is.mov
  • to-be render (using useSyncExternalStore)
to-be.mov

@gronxb gronxb changed the title feat: add usePagerView hook feat: add usePager hook Dec 19, 2023
@gronxb gronxb requested a review from troZee December 20, 2023 03:51
src/usePager.tsx Outdated
Comment on lines 58 to 60
if (Number(React.version.split('.')[0]) < 18) {
throw new Error('usePager requires React 18 or later.');
}
Copy link
Author

@gronxb gronxb Dec 28, 2023

Choose a reason for hiding this comment

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

Obviously, this code doesn't look very good.

If your library users are below react 18 and you don't have useSyncExternalStore, you can install use-sync-external-store for backwards compatibility and add "dependencies" inside this project's package.json (zustand already does that ).

Currently, react-native-pager-view is a zero-dependency. However, since the project doesn't emphasize zero-dependency, it already has a dependency on react, and finally, use-sync-external-store is an official library, it doesn't seem to be a problem.

Related Links
https://github.com/pmndrs/zustand/blob/main/package.json#L205
https://www.npmjs.com/package/use-sync-external-store

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