Refine use navigation param #20

benseitz · 2019-05-27T17:10:25Z

Introduces a useState like API for useNavigationParam as suggested in #19 and allows a fallback value as a second argument.

Theres also an updated and one added test as well as updated documentation and a new warning because of this breaking changes.

Currently the linter is failing because of a missing type for fallbackValue.
Since I'm not very fluent in typescript I'd couldn't find a solution that doesn't end in errors :/

FireyFly · 2019-05-28T07:28:02Z

Very neat! I'm not particularly well-versed in typescript either, so you probably tried this, but I would expect fallbackValue?: NavigationParams[T] (or something along those lines) to work.

Though, digging a bit to see what the type of getParam itself is, it seems the ts typings for it defines it as an overloaded function, with a single-argument case returning a nullable result, and with the two-argument case enforcing the result to be non-nullable.

Honestly, I wouldn't worry too much about the fallback value param--it does make sense I think to support it, since it matches what getParam does, but I think it's much less of a hassle to add later on compared to the "return pair of value and setter" change. Like, it's nice if we find a typing that works for it, but it's not the end of the world if we skip the fallback-value param for now.

Thanks a lot for writing up this PR btw! 😄

README.md

src/Hooks.ts

satya164 · 2019-05-28T09:12:53Z

src/Hooks.ts

+  const { getParam, setParams } = useNavigation()
+  return [
+    getParam(paramName, fallbackValue),
+    (newValue: NonNullable<NavigationParams[T]>) => setParams({ [paramName]: newValue }),


Maybe worth wrapping this in React.useCallback to preserve the reference?

Which part exactly?

The function which is used to update the param

I don't know if it's that much of a performance difference to just memoize the reference to that function.

It's not about performance optimisation due to memoizing, but it will avoid extra step to memoize it if you are using React.memo or React.PureComponent. iirc, the updater function from React.useState preserves the reference for this reason

benseitz · 2019-05-28T09:31:04Z

@satya164 Thank you for the review! I pushed some changes, that resolve most of the change requests and the failing linter :)

slorber · 2019-06-27T12:21:06Z

src/Hooks.ts

+  const { getParam, setParams } = useNavigation()
+  return [
+    fallbackValue ? getParam(paramName, fallbackValue) : getParam(paramName),
+    (newValue: NavigationParams[T]) => setParams({ [paramName]: newValue }),


Agree with @satya164, need useCallback here

const setParamValue = useCallback(() => { return (newValue: NavigationParams[T]) => setParams({ [paramName]: newValue }); },[setParams]);

Sorry for the delay - I just implemented it like you suggested :)

slorber · 2019-09-20T08:38:02Z

Thanks @benseitz , that looks fine to me

Can you rebase and resolve the conflicts?

I'm not going to merge it very soon because it's a breaking chance, will try to see what other breaking changes we can do for a v2

slorber · 2019-09-30T11:36:36Z

@satya164 do you plan to include such hook in react-navigation v5?

As mentionned here: #44
I think it's not a good idea to add hooks here that won't be in v5, will only make the migration more painful, and I currently don't see such a hook in v5 codebase)

satya164 · 2019-09-30T15:14:31Z

@slorber we are not, in v5 the route object is a top level prop, so instead of navigation.state.params, user can do route.params which is shorter.

For handling optional params and default value, the upcoming optional chaining and nullish coalescing operators work well:

- useNavigation().getParam('foo, 'defaultValue')
+ useRoute().params?.foo ?? 'defaultValue'

We do have a useRoute hook similar to useNavigation since the route isn't part of the navigation object anymore.

slorber · 2019-10-01T15:15:38Z

so, @satya164 do you think we should merge it? I think it's not a big deal even if v5 does not have it, because we already have useNavigationParam() anyway. The other option would be to remove it and ask user to use optional chaining, but not sure many have enabled it on their babel setup (TS just merged the PR)

satya164 · 2019-10-01T20:14:53Z

Regarding optional chaining, React Navigation v5 is still in alpha, so I'm not much worried about it not being available by default yet.

Regarding whether to merge the PR, it's upto you. Though a breaking change for this when upgrading to hooks v2 and again when upgrading to navigation v5 might not be best.

CyxouD

Thanks for your work 👍 Added fix for "Maximum update depth exceeded" crash and left suggestion to make Typescript users' life better :)

CyxouD · 2019-11-21T13:52:38Z

src/Hooks.ts

 export function useNavigationParam<T extends keyof NavigationParams>(
-  paramName: T
+  paramName: T,
+  fallbackValue?: NavigationParams[T]
 ) {
-  return useNavigation().getParam(paramName);
+  const { getParam, setParams } = useNavigation();
+  const setParamValue = useCallback(
+    () => {
+      return (newValue: NavigationParams[T]) =>
+        setParams({ [paramName]: newValue });
+    },
+    [setParams]
+  );
+  return [
+    fallbackValue ? getParam(paramName, fallbackValue) : getParam(paramName),
+    setParamValue(),
+  ];
 }


export default function useNavigationParam< T extends keyof Params, Params extends NavigationParams = NavigationParams >( paramName: T, fallback?: Params[T], ): [Params[T], (value: Params[T]) => boolean] { const { getParam, setParams } = useNavigation(); const setParamValue = useCallback( (newValue: Params[T]) => setParams({ [paramName]: newValue }), // `setParams` function is not memoized, so that it causes "Maximum update depth exceeded" (https://github.com/react-navigation/core/issues/71) // eslint-disable-next-line react-hooks/exhaustive-deps [paramName], ); // had to cast `paramName` because `navigation.getParam` thinks that some other type can be passed return [getParam(paramName as string, fallback), setParamValue]; }

I would suggest these change where I added additional optional type Params for NavigationParams, so that you can optionally determine actual types of keys' values instead of seeing any (it is made possible with Index types http://www.typescriptlang.org/docs/handbook/advanced-types.html#index-types). There is case when type is misleading, fallback value will always be shown as possibly undefined even if it is required in Params.
Also fixed "Maximum update depth exceeded" which described in suggestion below and added paramName to deps as ESlint suggested

CyxouD

Fix "Maximum update depth exceeded" crash in current implementation

CyxouD · 2019-11-21T15:27:55Z

src/Hooks.ts

+  const setParamValue = useCallback(
+    () => {
+      return (newValue: NavigationParams[T]) =>
+        setParams({ [paramName]: newValue });
+    },
+    [setParams]
+  );
+  return [
+    fallbackValue ? getParam(paramName, fallbackValue) : getParam(paramName),
+    setParamValue(),
+  ];


Both setParams in deps of useCallback (https://github.com/react-navigation/core/issues/71) and redundant function wrapping causes crash with "Maximum update depth exceeded" error in my React-Native app, so suggest to avoid it. Also added paramName prop as ESLint suggested

const setParamValue = useCallback( (newValue: NavigationParams[T]) => setParams({ [paramName]: newValue }), // `setParams` function is not memoized, so that it causes "Maximum update depth exceeded" (https://github.com/react-navigation/core/issues/71) // eslint-disable-next-line react-hooks/exhaustive-deps [paramName], ); return [ fallbackValue ? getParam(paramName, fallbackValue) : getParam(paramName), setParamValue, ];

slorber · 2020-02-03T18:22:59Z

Hi,

I think we'd better not ship this feature if this is not in v5

Thanks

benseitz added 4 commits May 27, 2019 19:01

Refine useNavigationParam

69657a7

Update test for refined useNavigationParam hook

06c0979

Update documentation for refined useNavigationParam hook

5130712

Add breaking changes warning to documentation

2c51afb

benseitz mentioned this pull request May 27, 2019

useNavigationParam: useState-like interface? #19

Open

satya164 reviewed May 28, 2019

View reviewed changes

README.md Outdated Show resolved Hide resolved

satya164 suggested changes May 28, 2019

View reviewed changes

benseitz added 2 commits May 28, 2019 11:25

Remove breaking changes warning from documentation

f456709

Improve typing in useNavigationParam hook

cec2c03

slorber suggested changes Jun 27, 2019

View reviewed changes

Wrap setParam in useCallback

ecb956b

benseitz requested review from slorber and satya164 September 12, 2019 17:34

CyxouD approved these changes Nov 21, 2019

View reviewed changes

CyxouD suggested changes Nov 21, 2019

View reviewed changes

slorber closed this Feb 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refine use navigation param #20

Refine use navigation param #20

benseitz commented May 27, 2019 •

edited

Loading

FireyFly commented May 28, 2019

satya164 May 28, 2019

benseitz May 28, 2019

satya164 May 28, 2019

benseitz May 28, 2019

satya164 May 28, 2019

benseitz commented May 28, 2019 •

edited

Loading

slorber Jun 27, 2019

benseitz Sep 12, 2019

slorber commented Sep 20, 2019

slorber commented Sep 30, 2019

satya164 commented Sep 30, 2019 •

edited

Loading

slorber commented Oct 1, 2019

satya164 commented Oct 1, 2019

CyxouD left a comment •

edited

Loading

CyxouD Nov 21, 2019 •

edited

Loading

CyxouD left a comment

CyxouD Nov 21, 2019

slorber commented Feb 3, 2020

Refine use navigation param #20

Refine use navigation param #20

Conversation

benseitz commented May 27, 2019 • edited Loading

FireyFly commented May 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benseitz commented May 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slorber commented Sep 20, 2019

slorber commented Sep 30, 2019

satya164 commented Sep 30, 2019 • edited Loading

slorber commented Oct 1, 2019

satya164 commented Oct 1, 2019

CyxouD left a comment • edited Loading

Choose a reason for hiding this comment

CyxouD Nov 21, 2019 • edited Loading

Choose a reason for hiding this comment

CyxouD left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slorber commented Feb 3, 2020

benseitz commented May 27, 2019 •

edited

Loading

benseitz commented May 28, 2019 •

edited

Loading

satya164 commented Sep 30, 2019 •

edited

Loading

CyxouD left a comment •

edited

Loading

CyxouD Nov 21, 2019 •

edited

Loading