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

Update react-native-vision-camera to the latest version #50650

Closed
hannojg opened this issue Oct 11, 2024 · 18 comments
Closed

Update react-native-vision-camera to the latest version #50650

hannojg opened this issue Oct 11, 2024 · 18 comments
Assignees
Labels
Monthly KSv2 Reviewing Has a PR in review Task

Comments

@hannojg
Copy link
Contributor

hannojg commented Oct 11, 2024

We are using quite an outdated version of react-native-vision-camera in expensify. There are two patches to make it work with the new arch, however none of these patches have been forwarded upstream yet.

We are using 4.0.0-beta.13, latest is 4.5.3

The latest version of react-native-vision-camera has lots of performance and stability improvements. Plus we have additional maintenance overhead due to the patches (and we are about to add a third one here)

Issue OwnerCurrent Issue Owner: @hannojg
@hannojg
Copy link
Contributor Author

hannojg commented Oct 11, 2024

cc @mountiny Is that something that we can work on?

@mountiny
Copy link
Contributor

@hannojg yeah lets do that, can we remove all the patches (merge the fixes upstream) as part of that?

@hannojg
Copy link
Contributor Author

hannojg commented Oct 14, 2024

Yes, thats the goal!

@mountiny
Copy link
Contributor

@hannojg can you breakdown the next steps here and what is the eta for them? Would someone else from Margelo work on it?

@hannojg
Copy link
Contributor Author

hannojg commented Oct 16, 2024

We've already started working on that one. As we have to consider new arch & old arch support, it might take a moment longer (its not just applying the patches we have in expensify, but making sure its compatible for all arch types).

Main work is happening here:

There are a few other PRs however that will be needed:

My ETA would be end of this week!

@mountiny
Copy link
Contributor

Ok thanks!

@hannojg
Copy link
Contributor Author

hannojg commented Oct 17, 2024

Short status update:

  • I have prepared the vision camera repo to be prepared for the upgrade (example app latest RN version, proper mono repo setup > otherwise we would get build errors later on when turning on new arch)
  • Working on the Fabric ComponentView

I currently ran into a bug with react-native-codegen, where it generates broken code (it must be a . instead of , here):

Screenshot 2024-10-17 at 18 03 45

For expensify this wasn't happening with the current patch as the codegen spec used in the patch is slightly incorrect.
I either have to fix this upstream in react native, or just ship a hard copy of the codegen files with VisionCamera (wouldn't like to do that; easy to break).

That being said, i am not sure if the ETA of EoW is realistic. I think more like beginning of next week!

@hannojg
Copy link
Contributor Author

hannojg commented Oct 18, 2024

Created an issue here, looking into it:

(The problem is that if this gets fixed in react native it will only be available in later versions. For expensify that could mean to just use another patch for reach native. However, its not really viable to ask all VisionCamera users to patch their react native. Due to that i am leaning towards shipping a fixed copy of the codegen code with VisionCamera for now)

@mountiny
Copy link
Contributor

agreed, the fixed copy sounds reasonable until this is fixed

@melvin-bot melvin-bot bot added the Overdue label Oct 21, 2024
@mountiny
Copy link
Contributor

going to make it weekly

@melvin-bot melvin-bot bot removed the Overdue label Oct 21, 2024
@mountiny mountiny added Weekly KSv2 and removed Daily KSv2 labels Oct 21, 2024
@hannojg
Copy link
Contributor Author

hannojg commented Oct 23, 2024

Update:

  • The iOS part is mostly done and working

Todo:

  • iOS fix remaining use cases (code scanner, FP)
  • Hard copy codegen code to avoid bug
  • bridge android

I am sure that i will finish those items by the end of this week, however Marc is currently OOO and I will need his final review (+ he is the only one who can do a release).
So with review + testing i would assume that this lands next week!

[Meanwhile i will open a PR that uses vision camera from the current development branch for testing in expensify!]

@mountiny
Copy link
Contributor

thanks for an update

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Weekly KSv2 labels Oct 29, 2024
@hannojg
Copy link
Contributor Author

hannojg commented Oct 29, 2024

Started a PR here:

iOS part is ready, now fixing up the android part

Once everything is ready we will test some more and publish a new version that we then can use in newdot!

@hannojg
Copy link
Contributor Author

hannojg commented Oct 29, 2024

Okay, android is working as well now. Waiting for @mrousavy to come back from OOO to finalise tmrw!

@hannojg
Copy link
Contributor Author

hannojg commented Nov 5, 2024

We have come to the conclusion that the code we need to introduce to vision camera for FabricComponents using codegen doesn't meet our quality barrier.
Proper new arch support for vision camera will be added when we migrate it to nitro modules.

(See discussion here)

@chrispader
Copy link
Contributor

@hannojg just to let you know, others want to upgrade VisionCamera in another PR: #51761 (comment)

@melvin-bot melvin-bot bot removed the Weekly KSv2 label Nov 29, 2024
Copy link

melvin-bot bot commented Nov 29, 2024

This issue has not been updated in over 15 days. @hannojg, @mountiny eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@melvin-bot melvin-bot bot added the Monthly KSv2 label Nov 29, 2024
@mountiny
Copy link
Contributor

I think we can close this now, we just updated the library

Once the library will be migrated to nitro we can handle that in a specific issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Monthly KSv2 Reviewing Has a PR in review Task
Projects
None yet
Development

No branches or pull requests

3 participants