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 repo and example to React Native 0.73 #879

Merged
merged 12 commits into from
Apr 24, 2024

Conversation

Saadnajmi
Copy link
Contributor

@Saadnajmi Saadnajmi commented Apr 4, 2024

Summary

Update the library and example app in this repo to React Native 0.73, which allows us to add a visionOS example app. As of #888 , dateTimePicker now has a minimum React Native version of 0.73. This PR will update the rest of the repo to reflect that.

To do this, I did:

  1. Use @rnx-kit/align-deps to update the package to React Native 0.73 (minimum required for visionOS support)
  2. Update the version (and template files) of react-native-test-app used to the latest major release
  3. As a consequence, E2E tests no longer work on Android new arch, so I disabled the CircleCI test for it. We already do this for iOS as well.

Test Plan

visionOS test app boots:

Screenshot 2024-04-04 at 4 35 13 PM

iOS test app for me boots as well. I didn't test Android or Windows.

What's required for testing (prerequisites)?

Xcode 15.2

What are the steps to reproduce (after prerequisites)?

Just run pod install and run the visionOS test app as you would an iOS test app

Compatibility

OS Implemented
iOS ✅ ❌
visionOS ✅ ❌
Android ✅ ❌

Checklist

  • I have tested this on a device and a simulator
  • I added the documentation in README.md
  • I updated the typed files (TS and Flow)
  • I added a sample use of the API in the example project (example/App.js)
  • I have added automated tests, either in JS or e2e tests, as applicable

@Saadnajmi
Copy link
Contributor Author

The second lock file (example/yarn.lock) can be removed if we update to yarn 4 and use yarn workspaces. Yarn 1 workspaces requires the root package to be private, which doesn't work for our case. Would there be interest in upgrading? I can make a PR if so.

@vonovak
Copy link
Member

vonovak commented Apr 5, 2024

hello @Saadnajmi and thank you for the PR!
I'll be happy to merge this (though this PR touches some of the same files as #878 - and right now I'm not sure which PR will be ready and merged first.)

I just want to make sure of one thing - I want to be able to run the example app, and have hot reloading working when I make edits in the src folder. Right now I'm not sure if metro will pick up those changes, can you pls double check that?

Thank you

@Saadnajmi
Copy link
Contributor Author

hello @Saadnajmi and thank you for the PR! I'll be happy to merge this (though this PR touches some of the same files as #878 - and right now I'm not sure which PR will be ready and merged first.)

I just want to make sure of one thing - I want to be able to run the example app, and have hot reloading working when I make edits in the src folder. Right now I'm not sure if metro will pick up those changes, can you pls double check that?

Thank you

Hey, sure I'll check that out. It seems I have a couple of things to check out, as the CI is failing.
A couple of questions:

  1. This PR is large mostly because it updates the test app and moves the dependencies around to accommodate. However, to add visionOS support, we actually only need to change the one line in the podspec (No changes to native or JS source were needed). If you prefer, I could just split that update out to its own PR? I normally try to pair updates with a test app change as well, but wanted to double check.
  2. Are we OK with the "extra" updates of adding RNX-Kit configs (to make RN update easier) and potentially using Yarn 4? I can definitely remove those changes, though keeping with yarn 1 does mean I'll either need to edit the CI pipelines or the test app more to keep everything building.

@vonovak
Copy link
Member

vonovak commented Apr 6, 2024

Hello,
I'm fine with having the changes in one PR.
Using yarn 4 is also good.

With rnx kit, can you point me to some docs around what specifically the things you added (metro config it seems?) do? I haven't used that before so unless there's some really good benefits I would probably prefer to not have it so that me or anyone else doesn't need to understand one more thing to contribute. TY.

Luckily the CI is fairly solid in this package so I trust that when it's green everything works.

@Saadnajmi Saadnajmi force-pushed the visionos branch 3 times, most recently from dabffe1 to 771fc59 Compare April 10, 2024 10:55
@vonovak
Copy link
Member

vonovak commented Apr 10, 2024

@Saadnajmi this will be a shot in the dark, but would perhaps MSFT be interested in sponsoring the maintenance of the module?
I have to say that I don't use windows so I couldn't be too active on the development side, but for things like keeping RN up to date, CI green and doing the necessary changes that always show up from time to time (now bridgeless mode), I think I could save you time there 🙂.

@Saadnajmi
Copy link
Contributor Author

Saadnajmi commented Apr 12, 2024

@vonovak Sorry, I asked around a bit, and I don't think we're at a point where we can sponsor repos at this time 🙁.

Meanwhile, I'm happy to spend time submitting PRs to fix issues / do some maintenance as they come up for stuff we care about (currently, that's adding visionOS support to several repos). Speaking of, I do hope the extra changes help with the maintenance, since adding visionOS support was really just a one line change but I figured it's worth trying to make the world better while we're at it 🙂 . For the windows development / maintenance, feel free to ping me or other Microsoft people and I can redirect to someone who does RN Windows dev (not me because I only really do Apple 😀 ).

For this PR, I need to keep working on making the CI green. You can probably ignore till that's done and/or I ping you about it, does that work?

@vonovak
Copy link
Member

vonovak commented Apr 12, 2024

Hey, thank you for the comment, works for me! 🙂 👍

@Saadnajmi
Copy link
Contributor Author

Saadnajmi commented Apr 17, 2024

EDIT: I ended up just incorporating @shwanton 's fix into this PR so that works!

Hey @vonovak I think I've fixed most of the issues, and the remaining one is that iOS is failing to build with the new architecture on 0.73 (which this PR updates the example app to). I see that @shwanton has a separate PR for that fix (#878) so I'll try and coordinate with him how we handle that. Two main questions:

  1. Adding visionOS support is literally just the 1 line in the podspec, Would you mind if I split that PR out into a separate one to quickly unblock? I'm happy to keep working on this PR for the rest of the improvements it brings to the repo, but I think "Add visionOS support" is now a misnomer :)
  2. Why is the Codegen generated code checked into the repo? Shouldn't that be generated by each end app? The build errors I am seeing, I assume is because 0.73 uses a newer version of Codegen. Is this because we have custom shadow node state?

@vonovak
Copy link
Member

vonovak commented Apr 17, 2024

Yes, feel free to split the PR up 🙂.

As for codegen-generated code, if you find that the currently generated code is the same as the one we have checked-in in the repo, then I guess there's no need to have it checked in. But I find it unlikely. There were some customizations made on it, I don't remember exactly what it was, maybe some intricacies related to layout or state as you're saying.

@Saadnajmi
Copy link
Contributor Author

Saadnajmi commented Apr 17, 2024

Yes, feel free to split the PR up 🙂.

As for codegen-generated code, if you find that the currently generated code is the same as the one we have checked-in in the repo, then I guess there's no need to have it checked in. But I find it unlikely. There were some customizations made on it, I don't remember exactly what it was, maybe some intricacies related to layout or state as you're saying.

@shwanton and I chatted offline about it. The repo adds custom state to the shadow note to track the date pickers' frame. That can't be replicated with just codegen atm afaik, so it needs to stay checked in. I may be wrong
PR for just the podspec change: #884

@Saadnajmi Saadnajmi changed the title Add visionOS support Add visionOS support & update test app to RN 0.73 Apr 17, 2024
@vonovak
Copy link
Member

vonovak commented Apr 17, 2024

Just got this message from circle:

In the last 30 days your team used the Free plan to test, build, and deploy for approximately 8752 minutes a day. That's a lot of building.

Without additional credits, your team will not be able to continue until the next refill on 05/22/24.

--

So in case building doesn't eun this might be the reason.

@Saadnajmi
Copy link
Contributor Author

Just got this message from circle:

In the last 30 days your team used the Free plan to test, build, and deploy for approximately 8752 minutes a day. That's a lot of building.

Without additional credits, your team will not be able to continue until the next refill on 05/22/24.

--

So in case building doesn't eun this might be the reason.

Ah, seems I have been going a bit too hard 😅 I can batch my updates better then. That does mean I probably won't revisit till 5/22/24.

@Saadnajmi Saadnajmi changed the title Add visionOS support & update test app to RN 0.73 Update to RN 0.73 Apr 17, 2024
package.json Show resolved Hide resolved
^ This is the 1st commit message:

Update RNTA to 0.73

^ The commit message react-native-datetimepicker#2 will be skipped:

^ update RNTA
@Saadnajmi Saadnajmi mentioned this pull request Apr 23, 2024
5 tasks
@Saadnajmi Saadnajmi changed the title Update to RN 0.73 Update example to React Native 0.73 Apr 24, 2024
@Saadnajmi Saadnajmi changed the title Update example to React Native 0.73 Update repo and example to React Native 0.73 Apr 24, 2024
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
Saadnajmi and others added 5 commits April 24, 2024 09:01
Co-authored-by: Vojtech Novak <vonovak@gmail.com>
Co-authored-by: Vojtech Novak <vonovak@gmail.com>
Co-authored-by: Vojtech Novak <vonovak@gmail.com>
Copy link
Member

@vonovak vonovak left a comment

Choose a reason for hiding this comment

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

🚀 🚀 🚀 🚀 🚀 🚀 🚀 🚀

@vonovak vonovak merged commit cf827f3 into react-native-datetimepicker:master Apr 24, 2024
5 checks passed
@Saadnajmi Saadnajmi deleted the visionos branch April 24, 2024 18:26
@vonovak
Copy link
Member

vonovak commented Apr 25, 2024

🎉 This issue has been resolved in version 8.0.0 🎉

If this package helps you, consider sponsoring us! 🚀

@Saadnajmi
Copy link
Contributor Author

🎉 This issue has been resolved in version 8.0.0 🎉

If this package helps you, consider sponsoring us! 🚀

Updating our rnx-kit preset :)
microsoft/rnx-kit#3112

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

Successfully merging this pull request may close these issues.

2 participants