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

Remove most or all definitions in navActions. #4417

Closed
chrisbobbe opened this issue Jan 15, 2021 · 1 comment · Fixed by #5408
Closed

Remove most or all definitions in navActions. #4417

chrisbobbe opened this issue Jan 15, 2021 · 1 comment · Fixed by #5408
Assignees

Comments

@chrisbobbe
Copy link
Contributor

chrisbobbe commented Jan 15, 2021

React Navigation 5, which we've recently upgraded to (#4296), discourages the NavigationService strategy we've been using, as we mentioned in 140c28c.

From their doc about that strategy:

Do not use this method when you have access to a navigation prop or useNavigation since it will behave differently, and many helper methods specific to screens won't be available.

And from their doc on upgrading from v4 to v5:

It's highly recommended to use the methods on the navigation object instead of using action creators and dispatch. It should only be used for advanced use cases.

We should aim to remove all the action creators in navActions and use, e.g., navigation.navigate instead. See discussion here and here.

The callsites for the action creators in navActions seem to fall into three categories:

  • We're in a React component that functions only as a "screen" component in React Navigation (i.e., the component is used in one way: it gets passed as the component prop to a <*.Screen /> component). Thus, it gets passed the navigation prop, and it's quite easy to just use it.
  • We're in a React component that doesn't function as a "screen" component, so it doesn't automatically get the navigation prop.
    • These can often easily be converted to Hooks-based components, so that useNavigation can provide the navigation object.
    • Sometimes, (as in the case of ComposeMenu) the conversion to Hooks would take a bit of effort. We can consider
      • just pushing through with the conversion,
      • passing down the navigation prop from the nearest component that has it, or
      • making a trivial Hooks-based wrapper component that uses useNavigation, in the same file, and export that.
  • We're not in a React component; e.g., we're somewhere deep in the event-handling code for a button being pressed on the action sheet, or for an outbound event coming from the WebView. It might actually end up being appropriate to keep the use of NavigationService for these callsites (as an "advanced use case"), rather than finding a way to thread the navigation object through from React. But we may find that something forces our hand.
@chrisbobbe chrisbobbe self-assigned this Jan 15, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 14, 2021
…pers.

navSelectors and navActions are kind of relics from before zulip#3804,
and we already have zulip#4417 for removing most or all of navActions.

Help along the removal of navSelectors by removing these things that
we stopped using in a recent commit.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 14, 2021
As mentioned in a previous commit, navSelectors and navActions are
kind of relics from before zulip#3804, and we already have zulip#4417 for
removing most or all of navActions.

Might as well remove navSelectors.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 26, 2021
…pers.

navSelectors and navActions are kind of relics from before zulip#3804,
and we already have zulip#4417 for removing most or all of navActions.

Help along the removal of navSelectors by removing these things that
we stopped using in a recent commit.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 26, 2021
As mentioned in a previous commit, navSelectors and navActions are
kind of relics from before zulip#3804, and we already have zulip#4417 for
removing most or all of navActions.

Might as well remove navSelectors.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 29, 2021
…pers.

navSelectors and navActions are kind of relics from before zulip#3804,
and we already have zulip#4417 for removing most or all of navActions.

Help along the removal of navSelectors by removing these things that
we stopped using in a recent commit.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 29, 2021
As mentioned in a previous commit, navSelectors and navActions are
kind of relics from before zulip#3804, and we already have zulip#4417 for
removing most or all of navActions.

Might as well remove navSelectors.
gnprice pushed a commit to gnprice/zulip-mobile that referenced this issue Sep 10, 2021
…pers.

navSelectors and navActions are kind of relics from before zulip#3804,
and we already have zulip#4417 for removing most or all of navActions.

Help along the removal of navSelectors by removing these things that
we stopped using in a recent commit.
gnprice pushed a commit to gnprice/zulip-mobile that referenced this issue Sep 10, 2021
As mentioned in a previous commit, navSelectors and navActions are
kind of relics from before zulip#3804, and we already have zulip#4417 for
removing most or all of navActions.

Might as well remove navSelectors.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Sep 10, 2021
…pers.

navSelectors and navActions are kind of relics from before zulip#3804,
and we already have zulip#4417 for removing most or all of navActions.

Help along the removal of navSelectors by removing these things that
we stopped using in a recent commit.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Sep 10, 2021
As mentioned in a previous commit, navSelectors and navActions are
kind of relics from before zulip#3804, and we already have zulip#4417 for
removing most or all of navActions.

Might as well remove navSelectors.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Mar 2, 2022
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Mar 2, 2022
chrisbobbe added a commit that referenced this issue Mar 3, 2022
We don't like using NavigationService (#4417), so this is nice to be
able to do.

Not *quite* NFC: if we somehow manage to have two consecutive
UserStatusScreens at the top of the stack, we'll now just pop one of
them instead of both, because we lose `navigateBack`'s special logic
with its `sameRoutesCount` value. But that logic is designed for
`ChatScreen` -- we do expect to have multiple `ChatScreen`s at the
top of the stack sometimes. We don't expect that with
`UserStatusScreen`s.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Mar 5, 2022
We don't like using NavigationService (zulip#4417), so this is nice to be
able to do.

Not *quite* NFC: if we somehow manage to have two consecutive
EmojiPickerScreens at the top of the stack, we'll now just pop one
of them instead of both, because we lose `navigateBack`'s special
logic with its `sameRoutesCount` value. But that logic is designed
for `ChatScreen` -- we do expect to have multiple of `ChatScreen`s
at the top of the stack sometimes. We don't expect that with
`EmojiPickerScreen`s.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Mar 8, 2022
We don't like using NavigationService (zulip#4417), so this is nice to be
able to do.

Not *quite* NFC: if we somehow manage to have two consecutive
EmojiPickerScreens at the top of the stack, we'll now just pop one
of them instead of both, because we lose `navigateBack`'s special
logic with its `sameRoutesCount` value. But that logic is designed
for `ChatScreen` -- we do expect to have multiple of `ChatScreen`s
at the top of the stack sometimes. We don't expect that with
`EmojiPickerScreen`s.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Mar 9, 2022
We don't like using NavigationService (zulip#4417), so this is nice to be
able to do.

Not *quite* NFC: if we somehow manage to have two consecutive
EmojiPickerScreens at the top of the stack, we'll now just pop one
of them instead of both, because we lose `navigateBack`'s special
logic with its `sameRoutesCount` value. But that logic is designed
for `ChatScreen` -- we do expect to have multiple of `ChatScreen`s
at the top of the stack sometimes. We don't expect that with
`EmojiPickerScreen`s.
gnprice added a commit to gnprice/zulip-mobile that referenced this issue Jun 10, 2022
React Navigation upstream has since the 5.x release been
discouraging the use of the strategy we call NavigationService.
See discussion at 140c28c and:
  zulip#4417
Instead, the recommendation is to use the `navigation` prop where
available, and failing that the `useNavigation` hook.

In this commit, we make that change for all sites where we were
using NavigationService in a function component that is a screen
in a navigator, and gets its own navigation prop.
@gnprice gnprice assigned gnprice and unassigned chrisbobbe Jun 10, 2022
gnprice added a commit to gnprice/zulip-mobile that referenced this issue Jun 10, 2022
This covers most of the `navigateToFoo` nav-action creators, but
sadly not all of them: as discussed in the previous commit where we
simplified away most of the call sites, there are still some that
remain in places where we still use NavigationService.  So for now
we keep the nav-action creators that are used there.

Fixes: zulip#4417
gnprice added a commit to gnprice/zulip-mobile that referenced this issue Jun 10, 2022
React Navigation upstream has since the 5.x release been
discouraging the use of the strategy we call NavigationService.
See discussion at 140c28c and:
  zulip#4417
Instead, the recommendation is to use the `navigation` prop where
available, and failing that the `useNavigation` hook.

In this commit, we make that change for all sites where we were
using NavigationService in a function component that is a screen
in a navigator, and gets its own navigation prop.
gnprice added a commit to gnprice/zulip-mobile that referenced this issue Jun 10, 2022
This covers most of the `navigateToFoo` nav-action creators, but
sadly not all of them: as discussed in the previous commit where we
simplified away most of the call sites, there are still some that
remain in places where we still use NavigationService.  So for now
we keep the nav-action creators that are used there.

Fixes: zulip#4417
gnprice added a commit to gnprice/zulip-mobile that referenced this issue Jun 14, 2022
React Navigation upstream has since the 5.x release been
discouraging the use of the strategy we call NavigationService.
See discussion at 140c28c and:
  zulip#4417
Instead, the recommendation is to use the `navigation` prop where
available, and failing that the `useNavigation` hook.

In this commit, we make that change for all sites where we were
using NavigationService in a function component that is a screen
in a navigator, and gets its own navigation prop.
@gnprice
Copy link
Member

gnprice commented Jun 14, 2022

This was done in #5408.

There are still some things remaining in navActions. Quoting from commit messages in that PR, ending with fc78777:

Still using NavigationService are […]

 * Sites that aren't directly on a React component.  We'll let these
   be for the present.

   Many of these (like src/webview/handleOutboundEvents.js, and action
   sheets) are straightforwardly invoked from particular components,
   so should probably just take a navigation object as a parameter.
   For others (like doEventActionSideEffects) it's less direct; but
   they're still ultimately invoked from some React component, so the
   same solution can be applied, just with a bit more plumbing.

--

This commit doesn't cover all the call sites of these `navigateToFoo`.
Where we're dispatching the result on `NavigationService` instead of
on a component's `navigation` prop (whether gotten directly, passed
down from a parent component, or gotten equivalently through
`useNavigation`), there is no `push` method, so in those places we
stick for now with `dispatch`.

--

nav [nfc]: Cut now-disused navActions action creators

This covers most of the `navigateToFoo` nav-action creators, but
sadly not all of them: as discussed in the previous commit where we
simplified away most of the call sites, there are still some that
remain in places where we still use NavigationService.  So for now
we keep the nav-action creators that are used there.

But 22 out of the 34 navActions action creators we had are now deleted.

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Sep 15, 2022
Further in the direction of zulip#5408, which switched most of our
then-existing uses of NavigationService to use `navigation` instead.

As noted there:

> Switching away from `NavigationService` is recommended by React
> Navigation upstream, as described at zulip#4417. We also now get
> type-checking on those `navigation.push` calls -- Flow is able to
> check that the route params we pass line up with what the route in
> question expects -- which wasn't/isn't the case for the
> implementations of the `navActions` functions.

This causes a few of the navActions functions to lose their last
remaining caller, so delete those.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Nov 17, 2022
Further in the direction of zulip#5408, which switched most of our
then-existing uses of NavigationService to use `navigation` instead.

As noted there:

> Switching away from `NavigationService` is recommended by React
> Navigation upstream, as described at zulip#4417. We also now get
> type-checking on those `navigation.push` calls -- Flow is able to
> check that the route params we pass line up with what the route in
> question expects -- which wasn't/isn't the case for the
> implementations of the `navActions` functions.

This causes a few of the navActions functions to lose their last
remaining caller, so delete those.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Nov 17, 2022
Further in the direction of zulip#5408, which switched most of our
then-existing uses of NavigationService to use `navigation` instead.

As noted there:

> Switching away from `NavigationService` is recommended by React
> Navigation upstream, as described at zulip#4417. We also now get
> type-checking on those `navigation.push` calls -- Flow is able to
> check that the route params we pass line up with what the route in
> question expects -- which wasn't/isn't the case for the
> implementations of the `navActions` functions.

This causes a few of the navActions functions to lose their last
remaining caller, so delete those.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Nov 17, 2022
Further in the direction of zulip#5408, which switched most of our
then-existing uses of NavigationService to use `navigation` instead.

As noted there:

> Switching away from `NavigationService` is recommended by React
> Navigation upstream, as described at zulip#4417. We also now get
> type-checking on those `navigation.push` calls -- Flow is able to
> check that the route params we pass line up with what the route in
> question expects -- which wasn't/isn't the case for the
> implementations of the `navActions` functions.

This causes a few of the navActions functions to lose their last
remaining caller, so delete those.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Nov 17, 2022
Further in the direction of zulip#5408, which switched most of our
then-existing uses of NavigationService to use `navigation` instead.

As noted there:

> Switching away from `NavigationService` is recommended by React
> Navigation upstream, as described at zulip#4417. We also now get
> type-checking on those `navigation.push` calls -- Flow is able to
> check that the route params we pass line up with what the route in
> question expects -- which wasn't/isn't the case for the
> implementations of the `navActions` functions.

This causes a few of the navActions functions to lose their last
remaining caller, so delete those.
BrandonNgoranNtam pushed a commit to BrandonNgoranNtam/zulip-mobile that referenced this issue Nov 22, 2022
Further in the direction of zulip#5408, which switched most of our
then-existing uses of NavigationService to use `navigation` instead.

As noted there:

> Switching away from `NavigationService` is recommended by React
> Navigation upstream, as described at zulip#4417. We also now get
> type-checking on those `navigation.push` calls -- Flow is able to
> check that the route params we pass line up with what the route in
> question expects -- which wasn't/isn't the case for the
> implementations of the `navActions` functions.

This causes a few of the navActions functions to lose their last
remaining caller, so delete those.
BrandonNgoranNtam pushed a commit to BrandonNgoranNtam/zulip-mobile that referenced this issue Nov 22, 2022
Further in the direction of zulip#5408, which switched most of our
then-existing uses of NavigationService to use `navigation` instead.

As noted there:

> Switching away from `NavigationService` is recommended by React
> Navigation upstream, as described at zulip#4417. We also now get
> type-checking on those `navigation.push` calls -- Flow is able to
> check that the route params we pass line up with what the route in
> question expects -- which wasn't/isn't the case for the
> implementations of the `navActions` functions.

This causes a few of the navActions functions to lose their last
remaining caller, so delete those.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants