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

feat: move test app to react-native-test-app #2746

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

Saadnajmi
Copy link
Contributor

@Saadnajmi Saadnajmi commented Nov 19, 2024

TODO:

  • Test iOS Paper
  • Test iOS Fabric
  • Test Android Paper
  • Test Android Fabric

Summary

I am interested in (eventually) adding macOS support to react-native-skia. Generally the first step to this is to first move the repository's test app to React Native Test App. This repo offers a few benefits:

  1. RNTA handles the native code of an example app for you (similar to expo's continuous native generation), so you only have to worry about your apps' JS.
  2. Backwards and forwards compatibility across a few Raect Native minor versions
  3. (Most importantly) The ability to easily add support for new platforms. In this case, of the 3 extra platforms RNTA supports ( macOS, visionOS, windows), the ones I know how to port after this change are macOS and visionOS.

Notes

  • I dropped the babel plugin transform-inline-environment-variables because things seemed to bundle fine without it?
  • I dropped a few other dependencies in the test app I didn't know what they were doing. Most likely I'll add a couple back as I try to get the CI to pass.

Test Plan

I got the iOS example app to run with the old architecture so far:

Screenshot 2024-11-18 at 9 28 08 PM

@Saadnajmi
Copy link
Contributor Author

@wcandillon Could I get the workflows approved so I can start testing my changes?

@wcandillon
Copy link
Contributor

@Saadnajmi I'm very excited about this and I will try to help as much as I can with it, however transform-inline-environment-variables cannot be dropped as we need it for CI (to open the app on the e2e test screen).

@Saadnajmi
Copy link
Contributor Author

@Saadnajmi I'm very excited about this and I will try to help as much as I can with it, however transform-inline-environment-variables cannot be dropped as we need it for CI (to open the app on the e2e test screen).

Makes sense! I should have left the PR in a draft state, my apologies. I figured I'd need to add back 95% of what was dropped as I ran into their use cases :)

@Saadnajmi Saadnajmi force-pushed the rnta branch 2 times, most recently from f9561a4 to 0579737 Compare December 21, 2024 06:02
@Saadnajmi
Copy link
Contributor Author

Saadnajmi commented Dec 22, 2024

Hi, I'll be trying to pick this up again over the next couple of weeks, with the aim to add macOS support. Fingers crossed 🤞.
@wcandillon could I get a workflow run approval?

@wcandillon wcandillon self-requested a review December 29, 2024 21:02
Copy link
Contributor

@wcandillon wcandillon left a comment

Choose a reason for hiding this comment

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

@Saadnajmi thank you for doing this and I am ready to merge this asap. Reanimated doesn't configure on the Android build however.

'transform-inline-environment-variables',
'react-native-reanimated/plugin',
Copy link
Contributor

Choose a reason for hiding this comment

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

this is unfortunately needed for the E2E testing but I can gladly look into it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah. Speaking of, could you approve the CI workflow so I can test this PR better?

@Saadnajmi
Copy link
Contributor Author

@Saadnajmi thank you for doing this and I am ready to merge this asap. Reanimated doesn't configure on the Android build however.

I'll take a look at Android, thanks for noticing! For this PR, you don't have an issue with moving the examples to a more brownfield approach with react-native-test-app, where the list is in the app.json and each example is presented separately? I can keep the original App.tsx with rn-screens intact so migrating "back" is easy.

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.

2 participants