Skip to content

Conversation

@thymikee
Copy link
Contributor

@thymikee thymikee commented Nov 27, 2023

Summary:

For out-of-tree platforms that share most of the code with some base platform (like visionOS is based on iOS) the current Platform abstraction doesn't fit nicely. Making "visionos" a distinct platform would mean we couldn't leverage from all the community code hidden behind Platform.OS === "ios" checks.

Building on top of existing Metro infrastructure (namely customResolverOptions), we'd like to propose a concept of a "Platform variant". Variant serves two purposes:

  1. This PR: allows OOT platforms to properly resolve calls to react-native module, while not making them a de-facto platform. Would apply to react-native-visionos proxying react-native -> react-native-visionos.

    It would also help the react-native-tvos platform as well to avoid remapping in package.json: "react-native": "npm:react-native-tvos@latest",.

  2. Future proposal: allows extending custom out-of-tree platform resolver to resolve:
    File.{platform}.{variant}.js -> File.{platform}.js -> File.js

This change originates from visionOS fork: callstack#32

Changelog:

[GENERAL] [ADDED] - Add support for resolving custom platform variant

Test Plan:

Added a unit test for metroPlatformResolver to illustrate the change

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Callstack Partner: Callstack Partner labels Nov 27, 2023
@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Nov 27, 2023
return (context, moduleName, platform) => {
let platformOrVariant: string | null =
// $FlowFixMe
context.customResolverOptions.variant || platform;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how to cope with this type, so I disabled the checker. Would love to resolve it properly
image

@matthargett
Copy link
Contributor

@rozele is this close to what you had in mind? we tried to fold in your most up to date thoughts :)

@kelset
Copy link
Contributor

kelset commented Nov 28, 2023

cc @tido64 / @Saadnajmi / @acoates-ms (in case it might be relevant for our platforms)

@thymikee
Copy link
Contributor Author

thymikee commented Nov 28, 2023

Appreciate if someone with OOT experience could have a look. However this is definitely not relevant for Windows in this form. Also likely not for macOS because:

  1. it's already defined as a platform and it would be breaking for existing users
  2. there's a bigger fork scope for macOS, compared to visionOS and tvOS

FYI, macOS could benefit from simplifying its CLI, which is unrelated to this change (but we'd like to contribute to it once we're done with integrating init properly with visionOS).

@thymikee
Copy link
Contributor Author

After discussing this topic with Metro team, we concluded that the Platform abstraction is already leaky, and that platform variant solution would be a band-aid to support our use case. It would also mean teaching RN users about "platform variant" and possibly cause confusion as to whether it's ".ios.visionos" or ".visionos.ios" and why.

To unblock ourselves, we're going to move to user space implementation (using user's metro.config.js) and not treating visionOS as a de-facto platform, so I'll close this PR. However, it may prove useful in the future, when we'll decide to revisit the Platform concept.

@thymikee thymikee closed this Nov 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Callstack Partner: Callstack Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants