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

Upgrade react-native to v0.71.2 #234

Closed

Conversation

priyeshshah11
Copy link
Contributor

@priyeshshah11 priyeshshah11 commented Feb 15, 2023

Details

This PR upgrades the react-native version to v0.71.2 & also upgrades the react-native-quick-sqlite to v8.0.0-beta.2 as that includes the v0.71.2 upgrade.

Related Issues

Expensify/App#15124

Automated Tests

No tests have been added as just upgrading the dependancy versions

Linked PRs

Expensify/App#15130 is dependent on this PR

@priyeshshah11 priyeshshah11 requested a review from a team as a code owner February 15, 2023 14:57
@github-actions
Copy link
Contributor

github-actions bot commented Feb 15, 2023

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@melvin-bot melvin-bot bot requested review from cristipaval and removed request for a team February 15, 2023 14:58
@priyeshshah11
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@priyeshshah11
Copy link
Contributor Author

@mountiny @roryabraham What's the best way to test this?

@priyeshshah11 priyeshshah11 changed the title upgrading react-native to v0.71.2 Upgrade react-native to v0.71.2 Feb 15, 2023
Copy link
Contributor

@cristipaval cristipaval left a comment

Choose a reason for hiding this comment

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

LGTM, but how can we test this? Shall we just merge it and let Applause test the App?

@mountiny
Copy link
Contributor

I think @marcaaron or @kidroca had some ways before how to test react-native-onyx with the App, do you have any tips here?

@marcaaron
Copy link
Contributor

marcaaron commented Feb 15, 2023

What is this related to? I see no linked issue. I see it now. Please update the description.

@marcaaron
Copy link
Contributor

One way to test this with the App repo is to just build the web dist with npm run build and then replace the entire react-native-onyx folder in node_modules.

@marcaaron
Copy link
Contributor

native doesn't need to build but web does IIRC.

@kidroca
Copy link
Contributor

kidroca commented Feb 16, 2023

Yes to what Marc suggests
I have been doing this for ages, because npm link does not work with metro the bundler react-native uses (it does not follow symlinks)

When I work on Onyx, and I want to test changes in App I do the following:

  • Use a tool that syncs my projects/Expensify/react-native-onyx to projects/Expensify/App/node_modules/react-native-onyx
  • Make changes in Onyx project, then run the Onyx build script (npm run build)

If this is for a one time test, you can just build the Onyx project and then copy it over to App/node_modules/
(You can also only copy dist and lib)
(Native doesn't have to build, it directly uses the code from lib)

@mountiny
Copy link
Contributor

@priyeshshah11 Do these steps sound good? We should be able to test it with the RN update PR then

@priyeshshah11
Copy link
Contributor Author

@mountiny yes the above mentioned steps worked & I was able to test web & it looks all good. Is there anything specific that I should be looking out for?

Thanks @kidroca & @marcaaron ❤️

@mountiny
Copy link
Contributor

@priyeshshah11 I think we just want to test usual from through the app:

  • create chat report
  • send a message
  • request money
  • Create workspace
  • send a message in the workspace chat
  • invite a member to a policy
  • upload profile picture

I think this should cover all the actions. Also SQLite is used in native device afaik so we should test this in web and also in native device 🙌

@priyeshshah11
Copy link
Contributor Author

Tested the above on web but still waiting on testing this on native, currently blocked by errors on Expensify/App#15130

@priyeshshah11
Copy link
Contributor Author

Finally, was able to fix the native build errors & have tested the above & it all looks good. I think we should now merge this & release & then perform thorough testing on Expensify/App#15130

Copy link
Contributor

@cristipaval cristipaval left a comment

Choose a reason for hiding this comment

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

I also tested and works well on my end.

@cristipaval
Copy link
Contributor

@priyeshshah11 it looks like the commits are not signed and we can't merge this one. Could you please check?

@priyeshshah11
Copy link
Contributor Author

@cristipaval I tried this to sign previous commits but I don't think that worked as expected. Do you want me to create a new branch and push the signed commit on that?

@priyeshshah11
Copy link
Contributor Author

@cristipaval Either way created #235 with signed commits, feel free to close #234 in favour of that.

cc: @mountiny

@cristipaval
Copy link
Contributor

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants