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.

Sign up for GitHub

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

Jump to bottom

chore(deps): migrate react-native-camera to the @interaxyz fork #6462

Closed
wants to merge 3 commits into from
Closed

chore(deps): migrate react-native-camera to the @interaxyz fork #6462

wants to merge 3 commits into from
+13 −33

Conversation

bakoushin
Copy link
Contributor

@bakoushin bakoushin commented Jan 31, 2025 •
edited
Loading

Description

Migrate react-native-camera to the @interaxyz fork in order to get rid of the patch.

The patch changes are implemented in the fork:
react-native-camera/react-native-camera@master...interaxyz:react-native-camera:master

Additional notes

This library is not maintained anymore, so we have to migrate to another one.

Possible alternatives (to consider in the future, as they aren't a drop-in replacement):

Test plan

  • CI
  • Tested manually on iOS device

Related issues

Backwards compatibility

Y

Network scalability

NA

bakoushin
bakoushin commented Jan 31, 2025
View reviewed changes
jest.config.js
@@ -41,6 +41,6 @@ module.exports = {
'^.+\\.(txt)$': require.resolve('./node_modules/react-native/jest/assetFileTransformer.js'),
},
transformIgnorePatterns: [
'node_modules/(?!@?react-native|@react-navigation|@react-native-community|uuid|statsig-js|@react-native-firebase|react-navigation|redux-persist|date-fns|victory-*|@walletconnect/react-native-compat|react-redux|@segment/analytics-react-native/node_modules/uuid)',
'node_modules/(?!@?react-native|@react-navigation|@react-native-community|uuid|statsig-js|@react-native-firebase|react-navigation|redux-persist|date-fns|victory-*|@walletconnect/react-native-compat|react-redux|@segment/analytics-react-native/node_modules/uuid|@interaxyz/react-native-camera)',
Copy link
Contributor Author

@bakoushin bakoushin Jan 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

otherwise unit tests are failing

@codecov Codecov
Copy link

codecov bot commented Jan 31, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.05%. Comparing base (8935871) to head (39339ff).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6462      +/-   ##
==========================================
- Coverage   89.05%   89.05%   -0.01%     
==========================================
  Files         730      730              
  Lines       31890    31890              
  Branches     6104     6104              
==========================================
- Hits        28401    28399       -2     
- Misses       3292     3294       +2     
  Partials      197      197
Files with missing lines Coverage Δ
src/qrcode/QRScanner.tsx 36.58% <100.00%> (ø)

... and 2 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8935871...39339ff. Read the comment docs.

@jeanregisser
Copy link
Member

Thanks, but do we really need the fork? AFAIK the existing patch is only needed for our unit tests to work in the framework.
Or would it also affect people consuming the framework?

@bakoushin
Copy link
Contributor Author

@jeanregisser I noticed the reason for a patch is to avoid app crash:

ViewPropTypes is no longer an export of react native but it was an import which caused the app to crash on the QR screen.

So I decided to keep it as a precaution. Happy to skip the fork if it's not the case. Maybe @kathaypacific could recall what it is?

@jeanregisser
Copy link
Member

jeanregisser commented Jan 31, 2025 •
edited
Loading

Ok cause we're currently able to run the Beefy app without this patch.
Or does it crash in the QR screen? 😅 👀

@bakoushin
Copy link
Contributor Author

I've built an iOS app without the patch. It doesn't crash on my device when I open QR screen/scan code.

@bakoushin
Copy link
Contributor Author

Unit tests are passing as well.

@jeanregisser
Copy link
Member

Oh nice! 💥

@bakoushin bakoushin mentioned this pull request Jan 31, 2025
Merged
@kathaypacific
Copy link
Collaborator

@jeanregisser I noticed the reason for a patch is to avoid app crash:

ViewPropTypes is no longer an export of react native but it was an import which caused the app to crash on the QR screen.

So I decided to keep it as a precaution. Happy to skip the fork if it's not the case. Maybe @kathaypacific could recall what it is?

sorry i don't remember the details here, but if the app does not crash on the QR screen then it's probably fine to remove! :D

@bakoushin
Copy link
Contributor Author

ok, then I'm going to get rid of the patch first, parking this PR

@bakoushin bakoushin closed this Jan 31, 2025
github-merge-queue bot pushed a commit that referenced this pull request Jan 31, 2025
@bakoushin
chore(deps): get rid of the react-native-camera patch (#6463)
f9f42bb 
### Description

This patch seems to not affect anything.

Context: #6462

### Test plan

* CI
* Tested manually on iOS device

### Related issues

- Related to RET-1308

### Backwards compatibility

Y

### Network scalability

NA
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bakoushin @jeanregisser @kathaypacific