Support Context api #4427

guyca · 2018-12-10T15:25:01Z

This PR adds improved support for React's Context api
As he playground app still used RN 51, I had to upgrade it first to RN 0.57.7 which turned out to be a bit of a challenge. I also took the opportunity to upgrade TypeScript to 3.2.

Context API

Navigation.registerComponent('navigation.playground.ContextScreen', () => (props) => (
  <TitleContext.Provider value={'Title from Provider'}>
    <ContextScreen {...props} />
  </TitleContext.Provider>
), () => ContextScreen);

Obviously, using static context works as well without needing to wrap the component.

Redux

Navigation.registerComponent('navigation.playground.ReduxScreen', () => (props) => (
  <Provider store={reduxStore}>
    <ReduxScreen {...props} />
  </Provider>
), () => ReduxScreen);

Plain Component

Navigation.registerComponent('navigation.playground.MyScreen', () => MyScreen);

This pattern supports any library requiring the user to wrap the base component.

ComponentWrapper.wrap was a static method which handled both regular components and components wrapped with Redux provider. This change is needed to support React’s Context api. Context api shares similarities with redux, as components are also wrapped with a Provider. The only difference is that the component can be wrapped with multiple providers.

As Context api is available since React 16.6.x, I need to upgrade the playground app to RN 0.57.5 which supports it. Hopefully this won’t take too long.

Took me only 2 days to update…

Instead, the default callback is always invoked - what am I missing… gods of TypeScript heed my call

henrikra · 2018-12-10T16:35:58Z

Hmm can you add more examples to PR description? For example how to use the new registerComponent with Redux? Also with nothing: just plain component. So can we get rid of all Redux with this PR?

Wrapping registered components can be done by returning a functional component. For example: ``` Navigation.registerComponent('navigation.playground.ContextScreen', () => (props) => ( <TitleContext.Provider value={'Title from Provider'}> <ContextScreen {...props} /> </TitleContext.Provider> )); ```

henrikra · 2018-12-11T09:50:19Z

And how the api is used if I just have plain component?

guyca · 2018-12-11T09:59:26Z

@henrikra As usual, no change.

henrikra · 2018-12-11T10:05:16Z

So basically we could deprecate all those redux wrappers? Aka we would not have any redux dependencies in this project?

guyca · 2018-12-11T10:25:12Z

Yes exactly. I already modified redux tests to this pattern, we can deprecate registerComponentWithRedux now and remove it in the next major version.

henrikra · 2018-12-11T11:17:55Z

This is good! Tell me when you are done so I can review this also! :)

guyca · 2018-12-11T15:01:13Z

@henrikra PR's actually ready - I'm just trying to figure out why our CI is giving me such hard time. For some reason Detox times out...

henrikra · 2018-12-11T16:03:19Z

@guyca Ok I can check it later today 👍

Meanwhile could you check my iOS PR? I really need your guys help and opinions on this #4434
This is very high prio for me at the moment ⚠️ I am grateful if you can review it 🙇

guyca · 2018-12-11T19:59:27Z

@henrikra Absolutely - we'll review it tomorrow

henrikra · 2018-12-11T20:17:46Z

Good to hear!

Hey since this PR is doing two things: can you specify files that has to be reviewed that are related to registerComponent? So people don't have to go through files that aren't related to that :D

henrikra · 2018-12-13T06:53:33Z

Looks good! On suggestions

I think we could get the API from this

Navigation.registerComponent('navigation.playground.ReduxScreen', () => (props) => (
  <Provider store={reduxStore}>
    <ReduxScreen {...props} />
  </Provider>
));

to this

Navigation.registerComponent('navigation.playground.ReduxScreen', (props) => (
  <Provider store={reduxStore}>
    <ReduxScreen {...props} />
  </Provider>
));

Aka removing one arrow function. If you look lib/src/components/ComponentWrapper.tsx line 20 the OriginalComponentGenerator just calls it self without params so I guess there is no point of having that extra function there? Wouldn't it be more clear without it se we can just directly pass those components?

guyca · 2018-12-13T12:30:40Z

@henrikra Thanks for reviewing!
I don't think that's change is possible as the arrow function returns a component, which in our case is a functional component.

Also, I had to change the api 😕

Navigation.registerComponent('navigation.playground.ContextScreen', () => (props) => (
  <TitleContext.Provider value={'Title from Provider'}>
    <ContextScreen {...props} />
  </TitleContext.Provider>
), ContextScreen);

The motivation is that we need to hoist statics from the actual component to the wrapping component in order to support static options. Do you have a better suggestion for the api?

henrikra · 2018-12-13T14:55:29Z

I think I have bit better idea. Could it be like this

Navigation.registerComponent('navigation.playground.ContextScreen', (WrappedScreenComponent) => (
  <TitleContext.Provider value={'Title from Provider'}>
    <WrappedScreenComponent />
  </TitleContext.Provider>
), MyComponentScreen);

So basically MyComponentScreen is my screen component which can be what ever. And WrappedScreenComponent what is coming back from wrap in ComponentWrapper. And then as you can see in the example I can wrap it what I want. If I just have plain component then api is like this:

Navigation.registerComponent('navigation.playground.ContextScreen', (WrappedScreenComponent) => <WrappedScreenComponent />, MyComponentScreen);

guyca · 2018-12-13T15:00:46Z

Navigation.registerComponent('navigation.playground.ContextScreen', (WrappedScreenComponent) => <WrappedScreenComponent />, MyComponentScreen);

This is a breaking change - we need to avoid breaking changes for now. The second argument needs needs to be a method which returns a Component, not an instance of a component.

henrikra · 2018-12-13T15:03:42Z

Okay what about same points as I said before but with extra function

Navigation.registerComponent('navigation.playground.ContextScreen', () => (WrappedScreenComponent) => (
  <TitleContext.Provider value={'Title from Provider'}>
    <WrappedScreenComponent />
  </TitleContext.Provider>
), MyComponentScreen);

Navigation.registerComponent('navigation.playground.ContextScreen', () => (WrappedScreenComponent) => <WrappedScreenComponent />, MyComponentScreen);

henrikra · 2018-12-13T15:08:31Z

Or what about this kind of API: we provide HOC from library like withNavigation

// in component file
import {withNavigation} from 'react-native-navigation';

class MyCoolComponent extends React.Component {}

export default withNavigation(MyCoolComponent);


// in screen file
import MyCoolComponent from './MyCoolComponent';
Navigation.registerComponent('navigation.playground.ContextScreen', MyComponentScreen);

What do you think about this? Yeah it is breaking but we can change function name so it is NOT breaking :D This feels most flexible to me. What do you think?

…entProvider This prevents premature require of the Component which impacts performance

guyca · 2018-12-16T07:54:25Z

Hey @henrikra, I think we should merge the feature now as I don't have much time to work on it anymore. Lets keep discussing the api in a new issue - we can consider this api experimental in the next few versions until we're both satisfied with it.

henrikra · 2018-12-16T08:34:18Z

Sounds good

The following PR introduces improved support for Context api and other api's which wrap the root view. ## Context api Navigation.registerComponent('navigation.playground.ContextScreen', () => (props) => ( <TitleContext.Provider value={'Title from Provider'}> <ContextScreen {...props} /> </TitleContext.Provider> ), () => ContextScreen); ## Redux Navigation.registerComponent('navigation.playground.ReduxScreen', () => (props) => ( <Provider store={reduxStore}> <ReduxScreen {...props} /> </Provider> ), () => ReduxScreen); ## Plain Component - not changed Navigation.registerComponent('navigation.playground.MyScreen', () => MyScreen); This PR also upgrades the TypeScript version to 3.2.0 and RN version used in the playground app to 0.57.7 * New Android build flavor - `reactNative57_7` * Unit test coverage is disabled, for some reason it broke after upgrading to RN 0.57.7

guyca and others added 7 commits December 2, 2018 14:17

Upgrade playground app to RN 0.57.1

6faa901

As Context api is available since React 16.6.x, I need to upgrade the playground app to RN 0.57.5 which supports it. Hopefully this won’t take too long.

Update playground app to RN 0.57.7

2890759

Took me only 2 days to update…

Tests are green + app is loading but coverage is messed up

5c39f1a

For some reason the wrapper callback isn’t invoked as expected

ec9e513

Instead, the default callback is always invoked - what am I missing… gods of TypeScript heed my call

f

b247849

Merge branch 'master' into wrapWithProviders

1b22340

guyca changed the title ~~Wrap with providers~~ Support Context api Dec 10, 2018

guyca added the platform: Android label Dec 10, 2018

guyca added this to the 2.3.0 milestone Dec 10, 2018

guyca added 5 commits December 10, 2018 18:14

f

019a7f2

f

188da1e

f

bd1eb09

Introduce RN 0.57.5 build flavor

d254fc9

Disable coverage threshold as it broke after updating to RN 0.57.7

d4d87a0

guyca mentioned this pull request Dec 10, 2018

Add support to pass any argument to <Provider /> #4335

Closed

guyca added 4 commits December 11, 2018 08:43

Use RN 0.57.7 typing files and fix android build script

9b66593

whoops

aa6484f

maybe now?

54eb71a

now?

b8b05fc

work

61b00c8

guyca added 2 commits December 11, 2018 13:44

android?

293b169

ios?

92f0af4

Publish artifacts

deda71d

guyca added 3 commits December 12, 2018 13:25

Hide context screen button

8a0b1e3

Fix invalid syntax after upgrading to TypeScript 3.2

7de1a7e

No need to spread props

31aeb77

guyca added 3 commits December 13, 2018 10:09

Fix SearchScreen e2e

1415ba9

Fix Search e2e (this time for real)

88a499b

Hoist statics to actual component to support static options

98a7bfe

guyca and others added 2 commits December 16, 2018 09:52

Provide the concrete Component class to registerComponent view Compon…

c770fbf

…entProvider This prevents premature require of the Component which impacts performance

Merge branch 'master' into wrapWithProviders

11e3b4f

Commit new api change

c5bdcf0

guyca merged commit 9d36521 into master Dec 16, 2018

guyca deleted the wrapWithProviders branch December 16, 2018 08:46

shuzootani mentioned this pull request Apr 9, 2019

[iOS] "Malformed calls from JS: field sizes are different." #4962

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Context api #4427

Support Context api #4427

guyca commented Dec 10, 2018 •

edited

Loading

henrikra commented Dec 10, 2018 •

edited

Loading

henrikra commented Dec 11, 2018

guyca commented Dec 11, 2018 •

edited

Loading

henrikra commented Dec 11, 2018

guyca commented Dec 11, 2018

henrikra commented Dec 11, 2018

guyca commented Dec 11, 2018

henrikra commented Dec 11, 2018

guyca commented Dec 11, 2018

henrikra commented Dec 11, 2018

henrikra commented Dec 13, 2018 •

edited by guyca

Loading

guyca commented Dec 13, 2018

henrikra commented Dec 13, 2018

guyca commented Dec 13, 2018

henrikra commented Dec 13, 2018

henrikra commented Dec 13, 2018 •

edited

Loading

guyca commented Dec 16, 2018

henrikra commented Dec 16, 2018

Support Context api #4427

Support Context api #4427

Conversation

guyca commented Dec 10, 2018 • edited Loading

Context API

Redux

Plain Component

henrikra commented Dec 10, 2018 • edited Loading

henrikra commented Dec 11, 2018

guyca commented Dec 11, 2018 • edited Loading

henrikra commented Dec 11, 2018

guyca commented Dec 11, 2018

henrikra commented Dec 11, 2018

guyca commented Dec 11, 2018

henrikra commented Dec 11, 2018

guyca commented Dec 11, 2018

henrikra commented Dec 11, 2018

henrikra commented Dec 13, 2018 • edited by guyca Loading

guyca commented Dec 13, 2018

henrikra commented Dec 13, 2018

guyca commented Dec 13, 2018

henrikra commented Dec 13, 2018

henrikra commented Dec 13, 2018 • edited Loading

guyca commented Dec 16, 2018

henrikra commented Dec 16, 2018

guyca commented Dec 10, 2018 •

edited

Loading

henrikra commented Dec 10, 2018 •

edited

Loading

guyca commented Dec 11, 2018 •

edited

Loading

henrikra commented Dec 13, 2018 •

edited by guyca

Loading

henrikra commented Dec 13, 2018 •

edited

Loading