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

Add react-navigation/native-stack to package.json #38427

Closed
chiragsalian opened this issue Mar 15, 2024 · 12 comments
Closed

Add react-navigation/native-stack to package.json #38427

chiragsalian opened this issue Mar 15, 2024 · 12 comments
Assignees
Labels
AutoAssignerAppLibraryReview Auto assign someone to review a new library being added to App Weekly KSv2

Comments

@chiragsalian
Copy link
Contributor

chiragsalian commented Mar 15, 2024

In order to properly evaluate if a new library can be added to package.json, please fill out this request form. It will be automatically assigned someone from our review team that will go through and vet the library.

In order to add any new production dependency, it must be approved by the App Deployer team. They will evaluate the library and decide if it's something we want to move forward with or if other alternatives should be explored.

Note: This is only for production dependencies. While we don't want people to add packages to dev-dependencies willy-nilly, we recognize that there isn't as great of a need there to secure them.

Name of library: react-navigation/native-stack

Details

  • Link to package: https://reactnavigation.org/docs/native-stack-navigator/
  • Problem solved by using this package: Native Stack Navigator provides a way for your app to transition between screens where each new screen is placed on top of a stack. With this we will get native screen transitions (same experience and feel as native apps), that should be smoother too because they do not block the JS thread.
  • Number of stars in GH: 2.8k
  • Number of monthly downloads: 562,936 weekly downloads
  • Number of releases in the last year: 13
  • Level of activity in the repo: High
  • Alternatives:

Two alternatives right now are:

  • @react-navigation/stack - what we are currently using. Highly customizable solution, but doesn't provide a native feel&like experience. + it's sensitive to JS workload (i. e. if JS thread is busy, then app will be laggy since transitions are managed on JS level)

  • react-native-navigation - provides native navigation, but it's not well maintained, has many issues and the API is not compatible with react-navigation

  • Are security concerns brought up and addressed in the library's repo?

The native-stack is simply a bridge between react-navigation API and navigation primitives provided by react-native-screens, so it's very hard to introduce any vulnerabilities there. I've searched through github issues and didn't find any security issues.

  • How many dependencies does this lib use that will be brought into our code?

Only one:

image
  • What will the effect be on the bundle size of our code?

~58kb of unzipped code.

PR adding this library - #37891

@chiragsalian chiragsalian added Weekly KSv2 AutoAssignerAppLibraryReview Auto assign someone to review a new library being added to App labels Mar 15, 2024
Copy link

melvin-bot bot commented Mar 15, 2024

Triggered auto assignment to @Julesssss (AutoAssignerAppLibraryReview), see https://stackoverflowteams.com/c/expensify/questions/17737 for more details.

Copy link

melvin-bot bot commented Mar 15, 2024

New Library Review

  • Are all the answers in the main description filled out properly and make sense?
  • Who maintains the library and how well is it maintained?
  • How viable are the alternatives?
  • Should we build it ourselves instead?

Once these questions are answered, start a thread in #engineering-chat, ping the @app_deployers group, and call for a vote to accept the new library. Once the vote is complete, update this issue with the outcome and procede accordingly. Here is a sample post:

Hey @app_deployers,

There is a request to add a new library to App that we need to consider. Please look at this GH and then vote :+1: or :-1: on accepting this new library or not.

GH_LINK

@chiragsalian
Copy link
Contributor Author

cc @mountiny @kirillzyusko, i believe the next step of the process is reviewing the library before we can add it. Since you guys have more context of the library can you fill in the missing pieces of the description and questions so that we can start a discussion for evalutation.

@kirillzyusko
Copy link
Contributor

@chiragsalian I can not edit original post, so I'll add my input in this comment 👀

Description

Alternatives

Two alternatives right now are:

  • @react-navigation/stack - what we are currently using. Highly customizable solution, but doesn't provide a native feel&like experience. + it's sensitive to JS workload (i. e. if JS thread is busy, then app will be laggy since transitions are managed on JS level)
  • react-native-navigation - provides native navigation, but it's not well maintained, has many issues and the API is not compatible with react-navigation

Are security concerns brought up and addressed in the library's repo?

The native-stack is simply a bridge between react-navigation API and navigation primitives provided by react-native-screens, so it's very hard to introduce any vulnerabilities there. I've searched through github issues and didn't find any security issues.

How many dependencies does this lib use that will be brought into our code?

Only one:

image

What will the effect be on the bundle size of our code?

~58kb of unzipped code.

Questions

Are all the answers in the main description filled out properly and make sense?

Yes

Who maintains the library and how well is it maintained?

It's well maintained. The authors are engineers from Callstack.

How viable are the alternatives?

As I pointed out earlier - we already use stack and we've faced its limitation.

The migration to react-native-navigation is not an option because:

  • it'll take an unbelievable amount of time;
  • will bring many new issues;
  • not sure whether it's supported on web (so we anyway will have to combine several solutions)

Should we build it ourselves instead?

I don't think so. native-stack is a community package and created with tough collaboration between react-navigation and react-native-screens. So I think we just should use this package and patch it if we think there is a bug that should be fixes.

@mountiny
Copy link
Contributor

Thank you! I have updated the OP!

@Julesssss I believe this is ready

@Julesssss
Copy link
Contributor

Are all the answers in the main description filled out properly and make sense?
Who maintains the library and how well is it maintained?
How viable are the alternatives?
Should we build it ourselves instead?

These have all been answered above and it seems quite clear that we should go ahead with this.

The native-stack is simply a bridge between react-navigation API and navigation primitives provided by react-native-screens

I shared a voting thread here.

@Julesssss
Copy link
Contributor

@chiragsalian the vote passed, and we can look into cross platform solutions separately.

@WoLewicki
Copy link
Contributor

Dropping my 2 cents here 😅
As for JS stack:

it's sensitive to JS workload (i. e. if JS thread is busy, then app will be laggy since transitions are managed on JS level)

It's not entirely true since under the hood, it uses Animated with useNativeDriver set to true (see https://github.com/react-navigation/react-navigation/blob/31bb8a1d7d4bd7eddfff6a5f9f552951f5fc6caa/packages/stack/src/views/Stack/Card.tsx#L81), so the transitions are handled on UI thread. If it is laggy, it probably means that UI is probably busy also, and transitions with native-stack might be laggy too. Swiping back is handled by react-native-gesture-handler also connected with Animated, so it should work on UI too. Did you investigate if the scenarios with laggy navigation with JS stack resolve in no lags with native-stack?

As for native-stack:

It's well maintained. The authors are engineers from Callstack.

Native-stack along with react-native-screens is maintained by Software Mansion.
As for adding web implementation there, I think the only reasonable way to do it would be to copy the whole JS stack implementation for it. I see no other quick solution for it since there are just no platform primitives that we could use under the hood to make it feel "native" 😄 So creating createPlatformStackNavigator with different implementations feels like the way to go for now unfortunately.

Overall, I am up for using native-stack on mobile platforms, since we don't really use any of the "customizations", provided by JS stack, in the App (they mainly focus on changing the look of the header and having custom transitions, which App does not use afaik). My only request would be to wait for the new arch PR to be merged and then check if all flows still work correctly since we had some issues with new arch and native-stack in the past.

@kirillzyusko
Copy link
Contributor

It's well maintained. The authors are engineers from Callstack.

@WoLewicki Well, I though that entire react-navigation is maintained by Callstack (and I thought that native-stack is also supported by callstack - because when I've decided to compare implementations of native-stack from react-native-screens and react-navigation -> they were quite different. And actually version from @react-navigation/native-stack is deployed to npm. And this fact made me think that native-stack in react-navigation is kind of "independent" package).

@WoLewicki
Copy link
Contributor

Well, I though that entire react-navigation is maintained by Callstack

Yeah, no problem 😅 As you can see on the landing page, it is maintained by people from different companies/organizations.
image

because when I've decided to compare implementations of native-stack from react-native-screens and react-navigation -> they were quite different.

I know it can cause confusion 😄 The package from react-native-screens was made to work with @react-navigation v5 and due to changes in the core of that library, we decided that the JS part of native-stack, which is pretty small btw, will live in @react-navigation repo for easier integration. That's why the implementations differ now, since they are meant for different @react-navigation versions.

@kirillzyusko
Copy link
Contributor

@WoLewicki oh, yeah, it makes sense what you're saying 👍 Thank you for the explanation!

@mountiny
Copy link
Contributor

@WoLewicki Thank you for raising these, I will create an issue to investigate more in-depth what would have to be added to the native-stack to allow us to use it on web too so we have better idea and then you could help us see if there is a way forwards

I put the PR in hold for Fabric 🙇

The native stack definitely brings better feel to the app with the native transitions, from my personal testing I havent noticed lagging of the animations using the native stack, it decreased for me compared to the current implementation, however, that is not objective measure

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoAssignerAppLibraryReview Auto assign someone to review a new library being added to App Weekly KSv2
Projects
None yet
Development

No branches or pull requests

5 participants