Skip to content
This repository has been archived by the owner. It is now read-only.

feat: add useNavigationState hook #273

Merged
merged 1 commit into from Jan 18, 2020
Merged

Conversation

@satya164
Copy link
Member

satya164 commented Jan 18, 2020

Sometimes it's useful to get the current navigation state inside a screen. We have the dangerouslyGetState method for that. However, the problem with this method is that it won't trigger a re-render when it changes, so user cannot rely on it for rendering something.

This adds a 2 things:

  1. A state event similar to focus and blur that user can subscribe to
  2. A useNavigationState hook that takes a selector and returns part of the state

Internally useNavigationState subscribes to the state event to get the current navigation state.

I have also made it mandatory to pass a selector to useNavigationState. This makes it harder to accidentally get the whole navigation state, which will trigger a re-render every time anything changes, even if we don't care about the change. With a selector, we can tell which part we care about, and if that part didn't change, it won't trigger a re-render.

For example, to get the same functionality as the old isFirstRouteInParent method:

function MyComponent({ route }) {
  const isFirstRouteInParent = useNavigationState(state => state.routes[0] === route);

  // content
}
@satya164 satya164 requested a review from osdnk Jan 18, 2020
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 18, 2020

Codecov Report

Merging #273 into master will increase coverage by 0.09%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #273      +/-   ##
==========================================
+ Coverage      94%   94.09%   +0.09%     
==========================================
  Files          37       38       +1     
  Lines         851      864      +13     
  Branches      216      216              
==========================================
+ Hits          800      813      +13     
  Misses         42       42              
  Partials        9        9
Impacted Files Coverage Δ
packages/core/src/useFocusEvents.tsx 95.45% <ø> (ø) ⬆️
packages/core/src/useNavigationState.tsx 100% <100%> (ø)
packages/core/src/useNavigationBuilder.tsx 98.79% <100%> (+0.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 43d2c45...3395e53. Read the comment docs.

Sometimes it's useful to get the current navigation state inside a screen. We have the `dangerouslyGetState` method for that. However, the problem with this method is that it won't trigger a re-render when it changes, so user cannot rely on it for rendering something.

This adds a 2 things:
1. A `state` event similar to `focus` and `blur` that user can subscribe to
2. A `useNavigationState` hook that takes a selector and returns part of the state

Internally `useNavigationState` subscribes to the state event to get the current navigation state.

I have also made it mandatory to pass a selector to `useNavigationState`. This makes it harder to accidentally get the whole navigation state, which will trigger a re-render every time anything changes, even if we don't care about the change. With a selector, we can tell which part we care about, and if that part didn't change, it won't trigger a re-render.

For example, to get the same functionality as the old `isFirstRouteInParent` method:

```js
function MyComponent({ route }) {
  const isFirstRouteInParent = useNavigationState(state => state.routes[0] === route);

  // content
}
```
@satya164 satya164 force-pushed the @satya164/use-navigation-state branch from 422754b to 3395e53 Jan 18, 2020
@satya164 satya164 merged commit 32a2206 into master Jan 18, 2020
5 checks passed
5 checks passed
Semantic Pull Request ready to be squashed
Details
ci/circleci: build-packages Your tests passed on CircleCI!
Details
ci/circleci: install-dependencies Your tests passed on CircleCI!
Details
ci/circleci: lint-and-typecheck Your tests passed on CircleCI!
Details
ci/circleci: unit-test Your tests passed on CircleCI!
Details
@satya164 satya164 deleted the @satya164/use-navigation-state branch Jan 18, 2020
@zmefz

This comment has been minimized.

Copy link

zmefz commented Jan 21, 2020

Hi @satya164. Thanks for this PR it is really helpful but there is one issue with it and I want to help you prevent future issues.
With current implementation if the selector change dynamically the result will not change until the next state change so the value won't match provided selector.
So here is my proposition (it might be a little bit messy but the idea should be clear):

function useNavigationState<T>(selector: Selector<T>): T {
  const navigation = useNavigation();
  const activeSelectorRef = useRef(selector);

  const [result, setResult] = React.useState(() =>
    selector(navigation.dangerouslyGetState())
  );

  // Recalculate result if selector has changed
  React.useEffect(() => {
    if (activeSelectorRef.current !== selector) {
      setResult(selector(navigation.dangerouslyGetState()));
      activeSelectorRef.current = selector;
    }
  }, [navigation, selector]);

  React.useEffect(() => {
    return navigation.addListener("state", e => {
      setResult(activeSelectorRef.current(e.data.state));
    });
  }, [navigation]);

  return result;
}

And here is how it should be used:

useNavigationState(React.useCallback(state => state.whatever, []))
// OR
useNavigationState(staticSelectorFunction)

I understand that my usage example looks worse than useNavigationState(state => state.whatever) but it should work more right and more in "React way" because it will not lead to inappropriate value

P.S. Sorry if the code have some errors: I didn't try it, just wanted to share the idea.

satya164 added a commit that referenced this pull request Jan 21, 2020
@satya164

This comment has been minimized.

Copy link
Member Author

satya164 commented Jan 21, 2020

@zmefz thanks for your comment. I think the simplest fix is to just re-run the selector on re-render so we always select the value from the latest state. I just pushed a commit doing it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.