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

Refactor upgrade command to just link to upgrade helper #1879

Merged
merged 1 commit into from
May 11, 2023

Conversation

blakef
Copy link
Contributor

@blakef blakef commented Mar 17, 2023

Summary:

The npx react-native upgrade command now points to the upgrade helper website which is the most practical way for users to upgrade their apps.

CleanShot 2023-03-17 at 23 06 43@2x

Why

Taking a table-stakes example, we should be able to upgrade from our template to each of our supported versions.

[SUCCESS] 0.69.8 → 0.70.7

Has android issues:

error Excluding files that failed to apply the diff:
  - android/app/build.gradle
  - android/app/src/main/jni/Android.mk

[FAIL] 0.70.7 → 0.71.4

[!] Invalid `Podfile` file: source is not valid JSON!.

Test Plan:

CI + ran a local build against a React Native project.

@thymikee
Copy link
Member

I've (re)wrote the upgrade command a while back and I'm in favor of that. Applying patches from remote repositories that resemble the local one—but not really—turned out to be quite unreliable. I believe that are more sophisticated ways to tackle this with more success, but I don't have the capacity to pursue it.

I remember at some point it worked for my use cases, but then https://github.com/react-native-community/rn-diff-purge changed the way the diffs are generated and it somehow broke it and I didn't really have the drive to pursue the real fix. There was no interest on fixing it from the community either.

So, let's do it. If we're afraid about pushback, because it maybe works for some cases, we can start by hiding the git patching behind a prompt and noticing that it's unreliable and will be removed soon.

@blakef blakef marked this pull request as ready for review April 20, 2023 16:20
@blakef
Copy link
Contributor Author

blakef commented Apr 20, 2023

I think moving away from this is the right approach, I'm really buoyed by RFC: SUpport Expo Config Plugins in React Native apps and should be spending more time supporting upgrading using this type of approach.

The upgrade helper has a high failure rate. Linking the user to the
upgrade helper is the best way to deprecate this feature.
@kelset
Copy link
Member

kelset commented May 3, 2023

any blockers in getting this merged?

@blakef
Copy link
Contributor Author

blakef commented May 3, 2023

Not from my side, I think it's GTG.

Copy link
Member

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

👍🏼 cc @douglowder since you contributed the tvOS part of the upgrade command.
I thought there's some kind of link to the react-native-tvos specific upgrade helper UI, but didn't find it. Maybe it's possible to put it next to macos and windows platforms like below?

image

@douglowder
Copy link
Member

@thymikee I created the TV-specific version of rn-diff-purge that actually contains the upgrade diffs -- however I never got around to creating a TV-specific UI.

@darkbasic
Copy link

The npx react-native upgrade command now points to the upgrade helper website which is the most practical way for users to upgrade their apps.

I don't agree with that. It is waaaay easier to git mergetool through the upgrade instead of manually updating files. Why did you do that?

@AdarshJais
Copy link

AdarshJais commented Jul 26, 2023

Is anyone else struggling to comprehend the reason behind this decision?

As far as I know, the purpose of the upgrade-helper website is to display the differences between versions, whereas the upgrade command is meant to apply those changes.

It seems that the recent changes in rn-diff-purge (https://github.com/react-native-community/rn-diff-purge) altered the way diffs are generated and consequently caused some issues.

@thymikee So, are you suggesting that the upgrade command relied on rn-diff-purge, and the changes made to rn-diff-purge rendered the upgrade command of no use? Is that the case?

Am I understanding It correctly or Am I missing something here???

Would also be interesting to know how expo is able to achieve it with expo update.

@thymikee
Copy link
Member

@AdarshJais it's not really the recent changes, they happened years ago. This command is heavily unreliable and works in, dunno, maybe 10% cases from my experience. That's why we decided to disable it and point to upgrade-helper directly

@TheSavior
Copy link
Contributor

To add some more details, our goal here as the group maintaining React Native and its community, is to help the community upgrade and stay on the latest version of React Native.

We've run surveys over the years about the biggest pain points people having with React Native, upgrades was always in the top 3 until recently. This command has existed since early on in React Native, trying to automatically patch existing projects. As people would make changes to their projects, these diffs wouldn't apply cleanly. As people navigate the git merge process they might miss some of those changes, getting them further and further out of date and making those diffs work even more infrequently. This is especially difficult in files that can't be meaningfully diffed and applied like the Xcode project. After any git merges to that file, projects frequently won't build anymore. This is what led to upgrades being a common complaint and the community recommending creating new projects and copying over all of your files.

The solution the community and maintainers built and supported was the upgrade helper. We found that people who manually investigated their diffs and could apply those patches in the right place, with guides for hard to merge files and inline comments about gotchas were much more successful. Since the creation of this tool, upgrades is no longer listed as one of the top issues.

The change in this PR is to increase the pit of success for the community by removing this common point of failure. We think people are best suited to upgrade using the helper (or using a framework like Expo).

@darkbasic
Copy link

darkbasic commented Aug 23, 2023

People who had issues clearly didn't were used to git mergetool. It's the same exact thing as using the upgrade helper but with three way merge and tools to assist you. A big downgrade in my opinion.

lawrence-forooghian added a commit to ably-labs/ably-js-react-native-example that referenced this pull request May 24, 2024
Motivated by the fact that the iOS version no longer builds in recent
Xcode versions (tried on 15.3.0).

Took the same approach as in 29e6994. (The `npx react-native upgrade`
tool doesn’t even _try_ and apply the diff any more; it just generates
the diff and you’re expected to try and apply it yourself [1].)

When re-applying 94b3770, note that the relevant documentation has been
moved to [2].

[1] react-native-community/cli#1879
[2] https://reactnative.dev/docs/set-up-your-environment?platform=android#installing-dependencies
lawrence-forooghian added a commit to ably-labs/ably-js-react-native-example that referenced this pull request May 24, 2024
Motivated by the fact that the iOS version no longer builds in recent
Xcode versions (tried on 15.3.0).

Took the same approach as in 29e6994. (The `npx react-native upgrade`
tool doesn’t even _try_ and apply the diff any more; it just generates
the diff and you’re expected to try and apply it yourself [1].)

When re-applying 94b3770, note that the relevant documentation has been
moved to [2].

[1] react-native-community/cli#1879
[2] https://reactnative.dev/docs/set-up-your-environment?platform=android#installing-dependencies
lawrence-forooghian added a commit to ably-labs/ably-js-react-native-example that referenced this pull request May 29, 2024
Motivated by the fact that the iOS version no longer builds in recent
Xcode versions (tried on 15.3.0).

Took the same approach as in 29e6994. (The `npx react-native upgrade`
tool doesn’t even _try_ and apply the diff any more; it just generates
the diff and you’re expected to try and apply it yourself [1].)

When re-applying 94b3770, note that the relevant documentation has been
moved to [2].

I’ve removed the yarn.lock file that `npx react-native init` now
generates.  Whilst the generated project supports both Yarn and NPM,
Andrew suggested that it’d be best to just support one package manager
(our README instructions only mention NPM) so that we don’t have to
commit two lock files [3].

[1] react-native-community/cli#1879
[2] https://reactnative.dev/docs/set-up-your-environment?platform=android#installing-dependencies
[3] #7 (comment)
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.

None yet

9 participants