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

Make osx example utilise the common example app #3055

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

Conversation

latekvo
Copy link
Contributor

@latekvo latekvo commented Aug 21, 2024

Description

This PR makes the MacOS example utilise the common example app.
This change will allow for more in-depth testing of MacOS functionalities.

For now, it's still work in progress

Test plan

Copy link
Contributor

@m-bert m-bert left a comment

Choose a reason for hiding this comment

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

I hope it won't have much conflicts with #2919 😅

MacOSExample/package.json Outdated Show resolved Hide resolved
@latekvo
Copy link
Contributor Author

latekvo commented Aug 22, 2024

I hope it won't have much conflicts with #2919 😅

I hope there won't be any, App.tsx should remain minimally changed with just a couple of styles differing, while the rest of the changes should be out of scope of #2919

Regardless, this PR is still WIP, and only works on my end due to some modifications to App.tsx structure, as I'm still working out some rendering inconsistencies, which are hopefully caused by default styling differences.

@latekvo
Copy link
Contributor Author

latekvo commented Aug 23, 2024

As of now, the common examples are working on MacOS, and I am still working on completely removing the MacOSExample directory, but that may prove challenging as currently we're relying on the babel.config.js to make the entire app work on MacOS, which unfortunately would prevent other platforms from using the examples if we were to naively marge both of these folders in the state that they are.

image

@m-bert
Copy link
Contributor

m-bert commented Sep 13, 2024

What has been done:

have a single app for both MacOS and the rest of the platforms

I think this is what we wanted to achieve 😅

What couldn't be done:

run both MacOS and iOS from a single xcode project
load MacOS example off the same metro instance as Android, iOS and Web runs off

First one is definitely not a problem. As for second, I have not looked deeply into this configuration, but before you had to start separate metro server for web, so it is also not a big deal.

I think the best course of action right now would be to revert progress back to somewhere around 02a96fa, and to cherry-pick some fixes that happened later, for example the build settings fix.

Sounds good 😄

@latekvo
Copy link
Contributor Author

latekvo commented Sep 13, 2024

PR functional as of 7717de6

currently only working on making the CI tests pass.

@latekvo latekvo requested a review from m-bert September 13, 2024 13:49
@latekvo latekvo marked this pull request as ready for review September 13, 2024 13:49
MacOSExample/macos/.xcode.env.local Outdated Show resolved Hide resolved
MacOSExample/package.json Outdated Show resolved Hide resolved
example/package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@j-piasecki
Copy link
Member

At the moment the example app on iOS doesn't build: https://github.com/software-mansion/react-native-gesture-handler/actions/runs/10941427525. (I've also triggered Android just to be sure: https://github.com/software-mansion/react-native-gesture-handler/actions/runs/10941919130).

Also, assets are not loading in the macOS app:
Screenshot 2024-09-19 at 15 19 55

I would assume this isn't expected, but I also wasn't following the PR that closely.

@latekvo
Copy link
Contributor Author

latekvo commented Sep 19, 2024

At the moment the example app on iOS doesn't build

I'm working on it. It's caused by presence of react-native-macos package in example, which we cannot remove just now.

Also, assets are not loading in the macOS app
I would assume this isn't expected, but I also wasn't following the PR that closely.

As far as I know, image assets are not available on macos.
I've tested this by trying to load png images into Image components, while their presence was detected by require, none showed up.
This is likely caused by asset linking issues on macos, i think the existing scripts have iOS paths hardcoded when they're trying to copy assets from project files to xcode Resources folder, but i'd have to doublecheck to be sure.
This is most likely the case for custom font assets, but I might be incorrectly associating image loading issues to the same problem.

I'll keep you updated.

(cc @j-piasecki)

@j-piasecki
Copy link
Member

As far as I know, image assets are not available on macos.
I've tested this by trying to load png images into Image components, while their presence was detected by require, none showed up.

Were the images under the example app or MacOSExample? This may be caused by the fact that you're effectively loading files from outside the app since it's not a monorepo.

Regarding failing Android build, there are multiple versions of @react-native/gradle-plugin in the project
Screenshot 2024-09-19 at 16 31 21
Vs on main:
Screenshot 2024-09-19 at 16 35 10
It may be related (it's possible that iOS failures are caused by a similar problem, though I don't remember any package that could cause it there from the top of my head).

@latekvo
Copy link
Contributor Author

latekvo commented Sep 20, 2024

Regarding failing Android build, there are multiple versions of @react-native/gradle-plugin in the project

Since these problems are caused by react-native and react-native-macos discrepancy, we'll have to wait for this issue to get resolved, it's blocking us from bumping react-native-macos to 0.74.

@latekvo
Copy link
Contributor Author

latekvo commented Sep 20, 2024

@j-piasecki Image loading fixed in 2d62e3a and 896f9be.

image
image

@latekvo
Copy link
Contributor Author

latekvo commented Sep 20, 2024

since 747bb71 iOS and Android should now both build successfully.
I've ran these two [Android, iOS] workflows just to be sure:

Test Android build Test iOS build

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.

3 participants