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

New Library Request: react-native-bundle-splitter #49519

Closed
kirillzyusko opened this issue Sep 20, 2024 · 14 comments
Closed

New Library Request: react-native-bundle-splitter #49519

kirillzyusko opened this issue Sep 20, 2024 · 14 comments
Assignees
Labels
AutoAssignerAppLibraryReview Auto assign someone to review a new library being added to App Daily KSv2

Comments

@kirillzyusko
Copy link
Contributor

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:

Details

  • Link to package: https://www.npmjs.com/package/react-native-bundle-splitter?activeTab=readme
  • Problem solved by using this package: fix: abracadabra page #48899
  • Number of stars in GH: 415
  • Number of monthly downloads: 10557
  • Number of releases in the last year: 2
  • Level of activity in the repo: Low
  • Alternatives: We can write a preloading mechanism ourself. It's just a matter of an additional code that will be present in Expensify codebase which we'll need to support.
  • Are security concerns brought up and addressed in the library's repo? No
  • How many dependencies does this lib use that will be brought into our code? 0
  • What will the effect be on the bundle size of our code? Size of the package is 20kb, but bundle will be increased by ~11kb
@kirillzyusko kirillzyusko added AutoAssignerAppLibraryReview Auto assign someone to review a new library being added to App Weekly KSv2 labels Sep 20, 2024
Copy link

melvin-bot bot commented Sep 20, 2024

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

Copy link

melvin-bot bot commented Sep 20, 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

@aimane-chnaif
Copy link
Contributor

cc: @dangrous

@melvin-bot melvin-bot bot added the Overdue label Sep 30, 2024
@grgia
Copy link
Contributor

grgia commented Oct 2, 2024

I haven't had a chance to look into this- @dangrous let me know if you have additional context. I'll take a look shortly

@melvin-bot melvin-bot bot removed the Overdue label Oct 2, 2024
@dangrous
Copy link
Contributor

dangrous commented Oct 8, 2024

Summary is, it allows for preloading of components:

1️⃣ preload the component in advance to be able to mount it synchronously on a demand
If we mount the component and the component is lazy and haven't been loaded yet, then when we switch navigator we remove old navigator, but a new one navigator doesn't have routes yet, so react-navigation clears a state, and when new routes are attached then a default route will be selected, because previous state was lost.

So we need to mount the screen/navigator synchronously.

@melvin-bot melvin-bot bot added the Overdue label Oct 10, 2024
@grgia
Copy link
Contributor

grgia commented Oct 14, 2024

I think we can 👍 this.

Alternatives: We can write a preloading mechanism ourself. It's just a matter of an additional code that will be present in Expensify codebase which we'll need to support.

@kirillzyusko would this be time-consuming or hard to build on our own?

@melvin-bot melvin-bot bot removed the Overdue label Oct 14, 2024
@kirillzyusko
Copy link
Contributor Author

@grgia I don't think it'll be time consuming, but from the other side I don't see a lot of sense in re-implementation that code directly inside the Expensify app, because:

  • it will be an additional code that we need to support;
  • new code will be identical to what's present in react-native-bundle-splitter

So I would vote for adding the library and don't add additional code to the Expensify codebase if possible 🙃

@melvin-bot melvin-bot bot added the Overdue label Oct 22, 2024
@grgia
Copy link
Contributor

grgia commented Oct 28, 2024

@kirillzyusko makes sense to me, I'll bring this to the appdeployers team for a vote

@melvin-bot melvin-bot bot removed the Overdue label Oct 28, 2024
@grgia grgia added Daily KSv2 and removed Weekly KSv2 labels Oct 28, 2024
@melvin-bot melvin-bot bot added the Overdue label Oct 30, 2024
Copy link

melvin-bot bot commented Oct 31, 2024

@grgia Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@grgia
Copy link
Contributor

grgia commented Nov 1, 2024

@melvin-bot melvin-bot bot removed the Overdue label Nov 1, 2024
@grgia
Copy link
Contributor

grgia commented Nov 1, 2024

the library looks good, I'm just curious:
are there are alternative libraries?
Instead of just fixing the underlying issue, would it be worth asking
@Kiryl Ziusko
to do a pass of the entire app for other rendering time improvements?
When we document the usage in Readme, are there any gotchas or complexities we'll need to be aware of?
as
@Kiryl Ziusko
is the library owner maybe he can answer these

image

@kirillzyusko could you take a look at these points from a comment in slack?

@melvin-bot melvin-bot bot added the Overdue label Nov 4, 2024
@grgia
Copy link
Contributor

grgia commented Nov 4, 2024

Looks like we are not going to move forward with this library as there are discussions about closing the original issue. I'm going to close this out.

@grgia grgia closed this as completed Nov 4, 2024
@melvin-bot melvin-bot bot removed the Overdue label Nov 4, 2024
@kirillzyusko
Copy link
Contributor Author

@grgia just to confirm you are going to close the original issue #44600 right?

@kirillzyusko
Copy link
Contributor Author

are there are alternative libraries?

The other library I was thinking of is react-loadable. But this is web-only, so I thought it would make sense to use cross-platform library equivalent (react-native-bundle-splitter works on all platforms supported by RN).

Instead of just fixing the underlying issue, would it be worth asking
@Kiryl Ziusko
to do a pass of the entire app for other rendering time improvements?

Yeah, I can do it, but if the issue got closed, then I don't think it's worth to do that anymore?..

When we document the usage in Readme, are there any gotchas or complexities we'll need to be aware of?

I don't have we have any complixities you need to be aware of. At least when you add a library for web only (as it was done in PR) you don't need to change any metro configurations etc.
If you are adding it to the native (mobile) side, then most likely you will need to change metro config.

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 Daily KSv2
Projects
None yet
Development

No branches or pull requests

4 participants