react-navigation / rfcs

RFCs for changes to React Navigation
Other
88 stars 16 forks source link

Bind Context providers to navigators #79

Closed fjcaetano closed 4 years ago

fjcaetano commented 5 years ago

This RFC adds the option to bind a navigator and its flow to a Context provider.

slorber commented 5 years ago

Hi

It is already possible to add a provider / wrapper (context or not) to a single navigator in your tree. You need to create a custom navigator view on top of it and assign the router static attribute to it. And this way you have full control over your provider and can support updates if you want.

Not sure to understand what the workarounds and alternatives currently are (can you show code before/after this change?) , but for me those are not workarounds but the official recommended way

fjcaetano commented 5 years ago

@slorber thanks for the quick reply. Could you provide a link with an example of a custom navigator implementing contexts? Is it possible to reuse the built-in navigators behavior when creating a custom one?

The workaround I mentioned is wrapping the whole app in a provider, for instance. You shouldn't have to do it if you know your provider is constrained within certain boundaries.

satya164 commented 5 years ago

You can wrap your navigator in anything like this:

function CustomNavigator(props) {
  return (
    <MyContext.Provider>
      <MyNavigator {...props} />
    </MyContext.Provider>
  );
}

CustomNavigator.router = MyNavigator.router;

I'd certainly like better React-like pattern instead of this.

The limitations I see with this RFC are:

  1. The context provider is specified as a static config option, which isn't very useful, you can import a constant to do the same thing
  2. What if you want multiple context providers?
  3. The provider shown here is just a wrapper React component, there's nothing specific regarding context

As mentioned, the alternative is to wrap a larger part of the code in the context provider. This can cause multiple unexpected errors like unnecessary rendering of components that are not related to the context.

A context value update won't cause unrelated components to re-render. Also imo re-rendering a component isn't an unexpected error, it's just React.

fjcaetano commented 5 years ago

@satya164 your example worked perfectly for me. Thanks. I didn't realise navigators were regular components.

I agree there could be a better way to achieve this, and I understand the limitation of the API I proposed, but I'm not sure I understood the problem with it being a static config. In which scenarios does it fall short? Also, if not a static config, then how? Remember that the idea is that it should be wrapping the whole component, not screens.

satya164 commented 5 years ago

I'm not sure I understood the problem with it being a static config. In which scenarios does it fall short

There are several issues with static config which I want to address later. Regarding context, static config means it's not possible to update the context value (or update globally) which severely limits the usefulness of context.

Also, if not a static config, then how?

It should be possible to user it normally as a component like in other places. For other configuration, it should be possible to pass them as props etc. But it's a broader topic of revamping the API.

Remember that the idea is that it should be wrapping the whole component, not screens.

If it's static, I don't think it really matters if you are wrapping individual screens or the navigator as the context value cannot be changed locally for wrapped component. You could wrap individual screens in the same provider and it should give the same result.