Skip to content

Conversation

@douglowder
Copy link
Member

Summary:

The upgrade command does not currently work for Apple TV and Android TV projects, because they use react-native-tvos, a fork of the main repo. (Issue #1476) (react-native-tvos/react-native-tvos#224)

Solution:

  • Provide a version of https://github.com/react-native-community/rn-diff-purge that has the diffs for the TV repo
  • Make upgrade.ts generic by detecting whether the project is using a forked repo, and having an easily extendable list of forks that are known to work. This list includes the main repo as well.
type ForkNameType = 'react-native' | 'react-native-tvos';

const forks = {
  'react-native': {
    rawDiffUrl:
      'https://raw.githubusercontent.com/react-native-community/rn-diff-purge/diffs/diffs',
    webDiffUrl: 'https://react-native-community.github.io/upgrade-helper',
    dependencyName: 'react-native',
  },
  'react-native-tvos': {
    rawDiffUrl:
      'https://raw.githubusercontent.com/react-native-tvos/rn-diff-purge-tv/diffs/diffs',
    webDiffUrl: 'https://react-native-community.github.io/upgrade-helper',
    dependencyName: 'react-native@npm:react-native-tvos',
  },
};

All methods in upgrade.ts now refer to this structure in their implementations.

Test Plan:

The upgrade unit tests have been refactored so that the test implementations and common mocks are in a common module, allowing the main repo and TV repo to be tested separately with their repo-specific mocks.

The upgrade command does not currently work for Apple TV and Android TV projects, because they use `react-native-tvos`, a fork of the main repo. (Issue react-native-community#1476) (react-native-tvos/react-native-tvos#224)

Solution:
- Provide a version of https://github.com/react-native-community/rn-diff-purge that has the diffs for the TV repo
- Make `upgrade.ts` generic by detecting whether the project is using a forked repo, and having an easily extendable list of forks that are known to work.  This list includes the main repo as well.

```ts
type ForkNameType = 'react-native' | 'react-native-tvos';

const forks = {
  'react-native': {
    rawDiffUrl:
      'https://raw.githubusercontent.com/react-native-community/rn-diff-purge/diffs/diffs',
    webDiffUrl: 'https://react-native-community.github.io/upgrade-helper',
    dependencyName: 'react-native',
  },
  'react-native-tvos': {
    rawDiffUrl:
      'https://raw.githubusercontent.com/react-native-tvos/rn-diff-purge-tv/diffs/diffs',
    webDiffUrl: 'https://react-native-community.github.io/upgrade-helper',
    dependencyName: 'react-native@npm:react-native-tvos',
  },
};

```

All methods in `upgrade.ts` now refer to this structure in their implementations.

The upgrade unit tests have been refactored so that the test implementations and common mocks are in a common module, allowing the main repo and TV repo to be tested separately with their repo-specific mocks.
Copy link
Member

@grabbou grabbou left a comment

Choose a reason for hiding this comment

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

It's a very nice PR! Thank you for working on this. Having the ability to map custom forks is definitely useful.

I think this will become less popular in the future, as I guess the preferred way is to have a custom project and integrate with code nicely.

However, with the abstraction made within this PR, React Native Windows and other platforms may have an easy entry-point for providing their own diffs in the future too. CC: @acoates-ms @kelset how this is done atm?

@grabbou grabbou merged commit c895393 into react-native-community:master Oct 4, 2021
@kelset
Copy link
Member

kelset commented Oct 4, 2021

looping in @NickGerleman who might know too

@acoates-ms
Copy link
Contributor

Can react-native-tvos not use the same mechanism for declaring itself as a platform as react-native-windows and react-native-macos does? -- Which allows it to co-exist within a repo better. So you could have an app that targets react-native and react-native-tvos without having to use a fork of react-native?

Similarly the information for rawDiffUrl and webDiffUrl should be something thats included in the react-native.config.js for each package, that way the CLI remains extendable for any platform, without us having to add platform specific code into the CLI for all the RN platforms.

@douglowder
Copy link
Member Author

@acoates-ms :

Can react-native-tvos not use the same mechanism for declaring itself as a platform as react-native-windows and react-native-macos does? -- Which allows it to co-exist within a repo better. So you could have an app that targets react-native and react-native-tvos without having to use a fork of react-native?

The reason for the decision to make react-native-tvos a fork is discussed extensively in react-native-tvos/react-native-tvos#11 . That being said, there have been enough changes in the last two years (such as the release plan changes announced today with RN 0.66) and the ability to declare yourself a platform, that maybe it is worth revisiting that decision.

Similarly the information for rawDiffUrl and webDiffUrl should be something thats included in the react-native.config.js for each package, that way the CLI remains extendable for any platform, without us having to add platform specific code into the CLI for all the RN platforms.

I remember looking at the config structure and did consider adding those to it, but decided to try and restrict my changes just to the upgrade command, to lessen the possibility of regressions in the rest of the code. However I think your proposal makes more sense in the long term, and I'll have a look at that at some point in the future.

@acoates-ms
Copy link
Contributor

react-native-macos is also implemented as a fork of react-native -- since it too shares a large part of its code with the iOS implementation. But the end user still uses it as a seperate package. We fixed the cli to allow it to redirect all react-native imports to specific packages based on the platform being built.

@douglowder
Copy link
Member Author

@acoates-ms that is amazing, and I will take a look to see how you did that 👍

It would definitely be worth aligning so that all platforms do these things the same way

@acoates-ms
Copy link
Contributor

All you need is this line: https://github.com/microsoft/react-native-macos/blob/0fb16c6ee620e63e3cadc331a75208b1250dca7c/react-native.config.js#L67

Within react-native-macos there is a conditional there, since we allow react-native-macos to be run either as a seperate package, or as a fork of react-native. And maybe you would want to do the same with react-native-tvos.

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