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

Add web platform passthrough support #165

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

Conversation

scottmas
Copy link

@scottmas scottmas commented Oct 23, 2024

Added web pass through support for multi platform projects. Since multi platform support is a bit finicky, I also had to migrate the example app to Expo. The bundler acts somewhat differently on web as well, so I had to do a minimal patch for three.js due to a poorly named variable and a patch to react-native-web due to a missing method on the Image component. I will submit PRs to those soon.

I'm somewhat certain that I broke the build for ios/android on the example app with the change to Expo. But since I never was able to even get the old example app building, that's not terribly surprising to me.

I'm more than happy to pound my head a little more against the breaking build, but it's probably something relatively simple which I imagine you would be better suited to fixing. Let me know though if you don't have time and you want me to look into it.

Also note that I removed from source control the ios and android folders since those are generated on demand by Expo. Nowadays I personally prefer this approach since it makes upgrading RN versions far easier, but it does require some gymnastics to customize the native projects (e.g. Expo config plugins https://docs.expo.dev/config-plugins/introduction/). I have some experience with these so one possible solution to fixing the builds would be for you to hack on the generated folders and keep track of your changes, and I could then package the changes into an Expo config plugin.

Thank you so much for all you do! This library is amazing.

@wcandillon
Copy link
Owner

this is very exciting :) Should we try to merge RN Web support in the example app (or add a separate app for RN Web) as a separate PR and take it from there?

@scottmas
Copy link
Author

scottmas commented Oct 24, 2024

I think there's really three directions we could go. Let me know which you prefer.

  1. You take a whack at getting the example app to build again with the switch to Expo in this PR. Difficulty: Unknown
  2. We switch back to the old example app and do a home brewed multi-platform setup that supports the web. Difficulty: Medium
  3. We create a separate web only example app that only does a hello world webgpu screen. This would make it harder though to ensure new features in the library work across all platforms. Difficulty: Easy

@wcandillon
Copy link
Owner

I think that 3. would be good enough no?

@matthargett
Copy link

You could also contribute web support to React Native Test App, solving this problem for the wider community. I would personally not advocate for Expo-specific transitions that give less integration test signal for the broader RN community.

I assume that if Expo is interested in this module, they'll set up their own sample app and integration test CI like they've done in the past.

@scottmas
Copy link
Author

scottmas commented Oct 24, 2024

Just pushed a change reverting the changes to the primary example app and adding a new Expo example app for web. I actually did that first but thought you would prefer the combined flow - should have pushed first hahah 😂. That would be cool Matt but I'm not at all familiar with React Native Test App. Let me know if there's any other changes you need!

@scottmas
Copy link
Author

Any updates on this William? Is this the approach you'd like to proceed with?

@wcandillon
Copy link
Owner

What @matthargett suggests would of course be ideal (RN Web support for React Native Test App would also be big for us in the React of React Native Skia). But if this path is too much resources, I would be open to 3.

@scottmas
Copy link
Author

Just played around a bit with react-native-test-app, and I like the idea, but my initial efforts showed it to be a bit buggy. For example, just following their quickstart for ios blew up until I did a required brew install, and eventually failed with an obscure error. In addition, it doesn't currently support react-native-web, the documentation is pretty bad, and its native project plugins are extremely limited compared to what you can do with Expo native plugins. I understand that there's actually a decent amount of hate from people towards Expo becoming the new "default" in react-native, but their native config plugin system is quite well thought, and gives you near unlimited power to customize your project while still getting near painless RN upgrades. They also have official support for MacOS and VisionOS in addition to the web. Don't get me wrong, I'm actually not an Expo shill - they have a tendency to release before their libraries are actually fully stable - but I've yet to find a better solution.

Is the fear that Expo adds too many unnecessary customizations to the ejected native projects? Their ejected ios/android folders really are quite close to the "vanilla" react-native ones. For example, this is the directory structure for the ios folder that gets generated:

./ios
├── .gitignore
├── .xcode.env
├── Podfile
├── Podfile.properties.json
├── apps
│   ├── AppDelegate.h
│   ├── AppDelegate.mm
│   ├── Images.xcassets
│   │   ├── AppIcon.appiconset
│   │   │   ├── [email protected]
│   │   │   └── Contents.json
│   │   ├── Contents.json
│   │   └── SplashScreenBackground.imageset
│   │       ├── Contents.json
│   │       └── image.png
│   ├── Info.plist
│   ├── SplashScreen.storyboard
│   ├── Supporting
│   │   └── Expo.plist
│   ├── apps-Bridging-Header.h
│   ├── apps.entitlements
│   ├── main.m
│   └── noop-file.swift
└── apps.xcodeproj
    ├── project.pbxproj
    ├── project.xcworkspace
    │   ├── contents.xcworkspacedata
    │   └── xcshareddata
    │       └── IDEWorkspaceChecks.plist
    └── xcshareddata
        └── xcschemes
            └── apps.xcscheme

It's very similar to what you would expect to see for a standard ios project, and makes it very easy to understand and grok exactly what's going on. Compare this to the directory structure that gets created with react-native-test-app for ios after generating:

/Users/scottashton/projects/react-native-webgpu/apps/rnta/ios
├── .xcode.env
├── Podfile
├── Podfile.lock
├── RNTA.xcworkspace
│   ├── contents.xcworkspacedata
│   └── xcshareddata
│       └── swiftpm
│           └── configuration
└── build
    └── generated
        └── ios
            ├── FBReactNativeSpec
            │   ├── FBReactNativeSpec-generated.mm
            │   └── FBReactNativeSpec.h
            ├── FBReactNativeSpecJSI-generated.cpp
            ├── FBReactNativeSpecJSI.h
            ├── RCTModulesConformingToProtocolsProvider.h
            ├── RCTModulesConformingToProtocolsProvider.mm
            └── React-Codegen.podspec.json

Both are perfectly valid ios projects, but the former is far more idiomatic.

Like I said, I'm not an Expo shill and know there very well could be some benefits to react-native-test-app I'm just not familiar with, as well as downsides to using Expo that I'm also not familiar with. But before I create a PR for react-native-test-app I just really want to make sure it is the best path forward for react-native-wgpu (and of course react-native-skia)!

@wcandillon
Copy link
Owner

An expo project would be great but I couldn't get it to work in the context of a monorepo (despite the process being well documented by expo). Not sure if I might have been missing something.

@wcandillon
Copy link
Owner

@scottmas just to make sure we are on the same page, we don't want to change the example app but rather have a new expo app in the apps folder if possible. That way we will have amazing testing coverage.

@scottmas
Copy link
Author

scottmas commented Oct 29, 2024

Ahh shoot, my bad - didn't understand you just wanted me to integrate the web app so that it used the workspace node_modules. The PR currently stands with just a single example app built with Expo, building on all platforms.

So are you 100% sure you want to keep using react-native-test-app? Do you have any thoughts on the points I raised above? All the platforms are working with the latest, and since we're using Expo ejection, there's absolutely zero native code being checked into the example app, unlike with RNTA where the upgrade process to newer versions of RN will be a bit involved unless I'm mistaken.

Let me know though if I need to revert and I'm happy to do so. There may be some hiccups though in switching the example web app to use workspace node_modules due to the yarn deduping process causing some version changes on shared packages and breaking some tests.

@wcandillon
Copy link
Owner

Yes we need to keep react-native-test-app is the expo app you are looking to contribute could easily become our main testing app and then people who are looking at platforms like macOS or VisionPro can use react-native-test-app. Why couldn't we use both?

@wcandillon
Copy link
Owner

@scottmas thank you for looking into this and your willingness to contribute to this. I think that a Web passthrough will be very cool :) If you contribute the expo app, that's something we would be interested to use in RN Skia as well.

@scottmas
Copy link
Author

Let's get this figured out! I'm still a bit confused though. Can you add me on discord? My user handle is @scottmas

@tido64
Copy link

tido64 commented Oct 30, 2024

Hi folks,

Author of RNTA here. I just wanted to clarify a few things that have been said in this thread to clear up some misconceptions.

Compare this to the directory structure that gets created with react-native-test-app for ios after generating

Almost none of the files listed are checked in when using RNTA. Podfile and Podfile.lock are currently the only files committed in webgpu:

image

The .xcworkspace is a product of running pod install and the build folder is where @react-native-community/cli outputs artifacts when you run run-ios. Both of these are expected in a vanilla (non-Expo) RN app. They are not byproducts of using RNTA, nor does it try to change of any of it. They're both considered build artifacts, and as you can see above, neither are expected to be checked in.

unlike with RNTA where the upgrade process to newer versions of RN will be a bit involved unless I'm mistaken

RNTA's primary goal is to make upgrades and downgrades easy. As stated in the docs, the number of steps needed to upgrade to latest RN is literally three: https://github.com/microsoft/react-native-test-app/wiki/Updating#updating-react-native-in-an-rnta-app

Spoiler: Two of the steps are running yarn install and rebuilding the app. The hardest part is figuring out which dependencies to bump, and for that, there is even a tool that will help you.

RNTA goes to great lengths to be backwards compatible. In fact, it currently supports all the way back to 0.66: https://github.com/microsoft/react-native-test-app/wiki#react-native-versions

its native project plugins are extremely limited compared to what you can do with Expo native plugins

I do not entirely understand this statement. RNTA uses the same code that Expo does. In fact, the docs even say to add @expo/config-plugins: https://github.com/microsoft/react-native-test-app/wiki/Config-Plugins

All core plugins that Expo ships should work out-of-box. Obviously, RNTA does not support Expo specific plugins like withExpoPlist and the Podfile ones (because RNTA doesn't know what a Podfile.properties.json is).

That said, in RNTA, you should only need to use config plugins if you need to configure something that cannot be done in app.json. RNTA supports a number of commonly used props like entitlements and privacy manifest: https://github.com/microsoft/react-native-test-app/wiki/Manifest-(app.json)

For example, just following their quickstart for ios blew up until I did a required brew install, and eventually failed with an obscure error.

RNTA expects users to have gone through the required setup as outlined here: https://reactnative.dev/docs/set-up-your-environment

This page was more prominent before Expo became standard, so maybe that's something can be made clearer in our docs. If you had to brew install additional tools, we definitely want to address that. If you have any specific feedback, please feel free to open an issue. We do want the RNTA experience to be as smooth as possible.

My two cents about Expo vs RNTA: Not everyone uses (or can use) Expo. I think it's therefore important to have both Expo and non-Expo (e.g. RNTA) example apps to ensure we don't unintentionally exclude anyone. Of course, it is up to @wcandillon whether he thinks the maintenance cost is going to be worth it.

@wcandillon
Copy link
Owner

@tido64 Thank you for taking the time to look at this thread.
RNTA is/has been great for us. We are missing the Web platform support and this where @scottmas started to wonder about adding a new testing app on top of RNTA or contributing Web support to RNTA. If RNTA would have Web support, I would love to use it for Skia as well.

@tido64
Copy link

tido64 commented Oct 30, 2024

@tido64 Thank you for taking the time to look at this thread. RNTA is/has been great for us. We are missing the Web platform support and this where @scottmas started to wonder about adding a new testing app on top of RNTA or contributing Web support to RNTA. If RNTA would have Web support, I would love to use it for Skia as well.

I hear you. I often hear this from others as well, but I am fairly certain I am not the right person to pick a tech stack for web (there are so many to choose from 😅). We have an open issue on web support. I know you've already seen this, but I'll post it here for posterity: microsoft/react-native-test-app#812

Everything in my last comment there is still true today. If anyone with a strong opinion on what the web experience should be and would like to contribute, please reach out.

@scottmas
Copy link
Author

scottmas commented Nov 6, 2024

Hey there, I haven't forgotten about this PR! But I'm onboarding a new employee and things have gotten a bit hectic. Also, I'm honestly a bit unsure as to how to proceeed at this point since I'm not familiar with RNTA and its architecture and how I would need to structure any potential upstream PR to it adding web support. It's tough because Expo has become the defacto standard for web support in an RN project, and there's just a large lift to understanding the internals of Metro bundler in such a way as to create a bespoke web integration for RNTA. Honestly, if I had the time I would love to create such a bespoke solution even if only to understand it better myself, but I don't know if it's a realistic task for me to be able bite off right now.

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.

4 participants