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

fix: react-native-screens not building on visionOS #2210

Merged
merged 2 commits into from
Jun 26, 2024

Conversation

okwasniewski
Copy link
Contributor

@okwasniewski okwasniewski commented Jun 25, 2024

Description

This PR fixes React Native screens not building on visionOS. As this platform needs explicit imports for used modules

Changes

  • Fixed building RNSConvert.h for visionOS

Test code and steps to reproduce

Compile on visionOS

Checklist

@okwasniewski
Copy link
Contributor Author

Hey! What do you think about implementing visionOS CI to ensure new changes don't break existing platform support?

@tboba
Copy link
Member

tboba commented Jun 25, 2024

Hey @okwasniewski! Yeah, I think we could add a new action for that 👍

@okwasniewski
Copy link
Contributor Author

Okay sounds good, I can see that you already have many examples did you consider migrating examples to https://github.com/microsoft/react-native-test-app ? We are in the middle of the migration of all of our Callstack libraries.

It supports:

  • React Native macOS
  • React Native windows
  • iOS/Android
  • React Nativce visionOS

@tboba
Copy link
Member

tboba commented Jun 25, 2024

@okwasniewski hmm, could you send some repository of a library that has the example migrated to react-native-test-app? I've got some concerns about migrating to it, but before asking them I need to check if in my head these questions are valid 😄

@okwasniewski
Copy link
Contributor Author

@tboba There you go: callstack/repack#629

@okwasniewski
Copy link
Contributor Author

@tboba I think adding visionOS CI can be done in a separate PR, do you think we can merge this one?

@kkafar
Copy link
Member

kkafar commented Jun 26, 2024

I'll get in a way here a bit. Can we get a link to documentation where it states that explicit imports are required? How is this enforced? I can see it only being done via compiler option & I'm not aware of such one (I obviously don't know vast majority of them, but quick googling did not help).

To see the compiler error would also be useful.

I'm asking these questions cause there is possibility that the issue lies somewhere else.

@okwasniewski
Copy link
Contributor Author

@kkafar It's missing references for UISemanticContentAttributeForceRightToLeft / UISemanticContentAttributeForceLeftToRight which come from UIKit.

Technically you should always import the framework you are using but I guess for iOS it's linked anyway.

Here is a screenshot:

CleanShot 2024-06-26 at 12 33 12@2x


In file included from /Users/okwasniewski/workspace/SpatialAmbientSounds/node_modules/react-native-screens/ios/RNSConvert.mm:1:
/Users/okwasniewski/workspace/SpatialAmbientSounds/node_modules/react-native-screens/ios/RNSConvert.h:13:4: error: expected a type
   13 | + (UISemanticContentAttribute)UISemanticContentAttributeFromCppEquivalent:
      |    ^
/Users/okwasniewski/workspace/SpatialAmbientSounds/node_modules/react-native-screens/ios/RNSConvert.h:16:4: error: expected a type
   16 | + (UINavigationItemBackButtonDisplayMode)UINavigationItemBackButtonDisplayModeFromCppEquivalent:
      |    ^
/Users/okwasniewski/workspace/SpatialAmbientSounds/node_modules/react-native-screens/ios/RNSConvert.mm:6:4: error: expected a type
    6 | + (UISemanticContentAttribute)UISemanticContentAttributeFromCppEquivalent:
      |    ^
/Users/okwasniewski/workspace/SpatialAmbientSounds/node_modules/react-native-screens/ios/RNSConvert.mm:17:4: error: expected a type
   17 | + (UINavigationItemBackButtonDisplayMode)UINavigationItemBackButtonDisplayModeFromCppEquivalent:
      |    ^
/Users/okwasniewski/workspace/SpatialAmbientSounds/node_modules/react-native-screens/ios/RNSConvert.mm:11:14: error: use of undeclared identifier 'UISemanticContentAttributeForceRightToLeft'
   11 |       return UISemanticContentAttributeForceRightToLeft;
      |              ^
/Users/okwasniewski/workspace/SpatialAmbientSounds/node_modules/react-native-screens/ios/RNSConvert.mm:13:14: error: use of undeclared identifier 'UISemanticContentAttributeForceLeftToRight'
   13 |       return UISemanticContentAttributeForceLeftToRight;
      |              ^
/Users/okwasniewski/workspace/SpatialAmbientSounds/node_modules/react-native-screens/ios/RNSConvert.mm:22:14: error: use of undeclared identifier 'UINavigationItemBackButtonDisplayModeDefault'
   22 |       return UINavigationItemBackButtonDisplayModeDefault;
      |              ^
/Users/okwasniewski/workspace/SpatialAmbientSounds/node_modules/react-native-screens/ios/RNSConvert.mm:24:14: error: use of undeclared identifier 'UINavigationItemBackButtonDisplayModeGeneric'
   24 |       return UINavigationItemBackButtonDisplayModeGeneric;
      |              ^
/Users/okwasniewski/workspace/SpatialAmbientSounds/node_modules/react-native-screens/ios/RNSConvert.mm:26:14: error: use of undeclared identifier 'UINavigationItemBackButtonDisplayModeMinimal'
   26 |       return UINavigationItemBackButtonDisplayModeMinimal;
      |              ^
5 warnings and 9 errors generated.

There is nothing in the docs about the compiler being more strict when compiling for visionOS but that's the thing I've observed when working with this platform.

Copy link
Member

@kkafar kkafar left a comment

Choose a reason for hiding this comment

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

Yeah, I took a look now into RNSConvert.h and I'm surprised it works right now on iOS, as we do not import any UIKit related headers. I'm not aware of XCode injecting any symbols into translation units, but maybe it does 🤷🏻‍♂️

I need to lookup some documentation to figure out why this is the case 😄

I'll allow myself a liberty of reorganising the imports and I think we're good to go.

Thanks for this PR!

Edit: Will merge the PR after CI passes.

ios/RNSConvert.h Outdated Show resolved Hide resolved
@kkafar kkafar merged commit 1914eea into software-mansion:main Jun 26, 2024
5 checks passed
alduzy pushed a commit that referenced this pull request Jun 28, 2024
## Description

This PR fixes React Native screens not building on visionOS. As this
platform needs explicit imports for used modules

## Changes

<!--
Please describe things you've changed here, make a **high level**
overview, if change is simple you can omit this section.

For example:

- Updated `about.md` docs

-->

- Fixed building `RNSConvert.h` for visionOS

<!--

## Screenshots / GIFs

Here you can add screenshots / GIFs documenting your change.

You can add before / after section if you're changing some behavior.

### Before

### After

-->

## Test code and steps to reproduce

Compile on visionOS

<!--
Please include code that can be used to test this change and short
description how this example should work.
This snippet should be as minimal as possible and ready to be pasted
into editor (don't exclude exports or remove "not important" parts of
reproduction example)
-->

## Checklist

- [ ] Included code example that can be used to test this change
- [ ] Updated TS types
- [ ] Updated documentation: <!-- For adding new props to native-stack
-->
- [ ]
https://github.com/software-mansion/react-native-screens/blob/main/guides/GUIDE_FOR_LIBRARY_AUTHORS.md
- [ ]
https://github.com/software-mansion/react-native-screens/blob/main/native-stack/README.md
- [ ]
https://github.com/software-mansion/react-native-screens/blob/main/src/types.tsx
- [ ]
https://github.com/software-mansion/react-native-screens/blob/main/src/native-stack/types.tsx
- [ ] Ensured that CI passes

---------

Co-authored-by: Kacper Kafara <kacperkafara@gmail.com>
ja1ns pushed a commit to WiseOwlTech/react-native-screens that referenced this pull request Oct 9, 2024
…2210)

## Description

This PR fixes React Native screens not building on visionOS. As this
platform needs explicit imports for used modules

## Changes

<!--
Please describe things you've changed here, make a **high level**
overview, if change is simple you can omit this section.

For example:

- Updated `about.md` docs

-->

- Fixed building `RNSConvert.h` for visionOS

<!--

## Screenshots / GIFs

Here you can add screenshots / GIFs documenting your change.

You can add before / after section if you're changing some behavior.

### Before

### After

-->

## Test code and steps to reproduce

Compile on visionOS

<!--
Please include code that can be used to test this change and short
description how this example should work.
This snippet should be as minimal as possible and ready to be pasted
into editor (don't exclude exports or remove "not important" parts of
reproduction example)
-->

## Checklist

- [ ] Included code example that can be used to test this change
- [ ] Updated TS types
- [ ] Updated documentation: <!-- For adding new props to native-stack
-->
- [ ]
https://github.com/software-mansion/react-native-screens/blob/main/guides/GUIDE_FOR_LIBRARY_AUTHORS.md
- [ ]
https://github.com/software-mansion/react-native-screens/blob/main/native-stack/README.md
- [ ]
https://github.com/software-mansion/react-native-screens/blob/main/src/types.tsx
- [ ]
https://github.com/software-mansion/react-native-screens/blob/main/src/native-stack/types.tsx
- [ ] Ensured that CI passes

---------

Co-authored-by: Kacper Kafara <kacperkafara@gmail.com>
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