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

[v2] react-native-calendars performance v1 vs v2 #3432

Closed
ferrannp opened this issue Jun 25, 2018 · 6 comments
Closed

[v2] react-native-calendars performance v1 vs v2 #3432

ferrannp opened this issue Jun 25, 2018 · 6 comments

Comments

@ferrannp
Copy link

Issue Description

I am trying react-native-navigation v2 and one of the main reasons for it was to load heavy screens where react-navigation is not really good at it. If I would use the latest, then I'd need to use InteractionManager.runAfterInteractions and a loading spinner but I would like to avoid that.

I was trying the example of wix/react-native-calendars which uses react-native-navigation 1.1.205 and it works as expected, the transition starts immediately and when the content is loaded then it shows, see:

v1

I am trying the same approach with v2 but it seems to work different it has a delay and it shows the screen when the calendar is loaded already. See:

v2

I am wondering if I am doing something wrong or something has changed. Is there a way to achieve the same as v1 react-native-navigation / react-native-calendars ?


Environment

  • React Native Navigation version: 2.0.2376
  • React Native version: 0.56.0-rc.3
  • Platform(s) (iOS, Android, or both?): iOS (I guess Android too)
  • Device info (Simulator/Device? OS version? Debug/Release?): Simulator
@ferrannp ferrannp changed the title v1 vs v2 with react-native-calendars performance [v2] react-native-calendars performance v1 vs v2 Jun 25, 2018
@guyca
Copy link
Collaborator

guyca commented Jun 25, 2018

Hey @ferrannp
When pushing screens in v1 on iOS - RNN didn't wait for the react view to render, instead, RNN immediately pushed a controller with an opaque (white by default) background. On the one hand, the transition starts off immediately, but on the other hand, as seen in the first gif, there was a "white flash" when the screen is pushed.

In v2, we elected to push the screens after the react view is rendered. We felt this is preferable as there will always be a delay between the time a react view is created (in native), and when it is first rendered.
This delay can be mitigated by rendering an initial simple layout and executing work in the componentDidAppear lifecycle hook instead of in the Component's constructor or componentDidMount method.

We can make this behaviour opt-out, but I really don't think this is desired. I hope these issues will be resolved in the future by the changes detailed in this post, especially the following paragraph on the async nature of RN:

To make React Native more lightweight and fit better into existing native apps, this rearchitecture has three major internal changes. First, we are changing the threading model. Instead of each UI update needing to perform work on three different threads, it will be possible to call synchronously into JavaScript on any thread for high-priority updates while still keeping low-priority work off the main thread to maintain responsiveness. Second, we are incorporating async rendering capabilities into React Native to allow multiple rendering priorities and to simplify asynchronous data handling. Finally, we are simplifying our bridge to make it faster and more lightweight; direct calls between native and JavaScript are more efficient and will make it easier to build debugging tools like cross-language stack traces.

Once these changes are completed, closer integrations will be possible. Today, it's not possible to incorporate native navigation and gesture handling or native components like UICollectionView and RecyclerView without complex hacks. After our changes to the threading model, building features like this will be straightforward.

@ferrannp
Copy link
Author

Hey @guyca, thanks for your explanation.

This was the main reason I moved away from react-navigation. The transition was crazy slow. I could mitigate that with InteractionManager.runAfterInteractions, render a spinner and then render the calendar.

Now here in v2 I also tried similar stuff, so I render the calendar only when we hit componentDidAppear. However, it is slower to do it this way than in v1.

The v1 one way, of course we do not want a white flash but with the background colour you have, it looks ok to me in the first gif I posted. It is great for v2 that the approach is changing and I also hope those new RN changes will bring even more improvements to this.

However, it would be really nice to have v1 way as optional (like passing a parameter to push). I mean, I am pretty sure that wix/react-native-calendars has its problems too in terms of performance (like it could maybe do some render optimizations there). But to be honest if this is not fix in RN, I think having that optional way to do it like v1 was doing it, it would be nice. Not sure if there is something before componentDidAppear but the approach is definitely slower than in v1.

WDYT?

@DanielZlotin
Copy link
Contributor

That's a good idea, making the compromise of supporting opt-out of navigation waiting for react to finish rendering

@guyca
Copy link
Collaborator

guyca commented Jul 4, 2018

After further discussions, we've decided to make this feature opt-in. Meaning that screens will be pushed immediately and will no longer waiting for initial render.

To opt-in, enable it in push/showModal animations:

options: {
  animations: {
    push: {
      waitForRender: true // Default: false
    },
    showModal: {
      waitForRender: true // Default: false
    }
  }
}

@yogevbd yogevbd removed their assignment Jul 4, 2018
guyca added a commit that referenced this issue Jul 5, 2018
This commit introduces an option to control the way components are pushed into the stack.
When pushing components into a stack, they are pushed the moment they are measured.
If `waitForRender` is true, components will be pushed only after the initial render.

api:

```js
options: {
  animations: {
    push: {
      waitForRender: true
    }
  }
}
```

TODO:
* Refactor
* Add tests

Related to #3432
@InternetExplorer8
Copy link

InternetExplorer8 commented Jul 10, 2018

After migrating from 2.0.2354 to 2.0.2399, RNN will no longer wait for the react view to render (like v1).
Was there a breaking change in the meantime? We use a HOC for redux.

@guyca
Copy link
Collaborator

guyca commented Jul 10, 2018

As of 2.0.2401, both iOS and Android don't wait for render when pushing screens or showing modals.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment