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

feat: improve upgrade to use rn-diff-purge #176

Merged
merged 12 commits into from Feb 22, 2019

Conversation

Projects
None yet
7 participants
@thymikee
Copy link
Collaborator

thymikee commented Feb 16, 2019

Summary:

Upgrading experience is a bit harsh for React Native. We either need to install global react-native-git-upgrade and try that, or patch projects manually using tools like rn-diff-purge.

Since rn-diff-purge gets some recognition in RN core and website and it has an automated path of applying patches, I thought that it would be nice to use it in our upgrade command.

What the command does now:

  1. Determine version to upgrade and run some validation checks
  2. Fetch patch between current version and specified from rn-diff-purge and rename diff app to current project name
    1. If the patch is empty, install RN and its peer deps
    2. Exit with success
  3. Prepare for 3-way merge by creating temporary patch file and adding new git remote
  4. Apply patch with a fallback to 3-way merge, exclude package.json because it will always be failing
  5. Install RN and its peer deps
  6. Cleanup temporary files and git remote
  7. Exit with success

This PR is technically breaking change, so I'm cool with introducing a temporary new command or put old one behind a different name.

Test Plan:

Added tests with various scenarios.

Successfully upgrading to specified version:

screenshot 2019-02-16 at 22 22 24

Failed to fetch a non-existent diff:

screenshot 2019-02-16 at 22 22 49

No upgrade needed:

screenshot 2019-02-16 at 22 33 27

Dependency mismatch:

screenshot 2019-02-16 at 22 36 04

Upgrading to older version:

screenshot 2019-02-16 at 22 39 50

@thymikee thymikee force-pushed the feat/upgrade branch from b20974c to 4877602 Feb 16, 2019

@turnrye

This comment was marked as outdated.

Copy link
Member

turnrye commented Feb 16, 2019

Maybe a flag to switch to this as an experimental new version of react-native upgrade, e.g. react-native upgrade --nextgen?

@TheSavior
Copy link
Contributor

TheSavior left a comment

This looks awesome and I’m excited to make this experience better for people. I’ll defer to the typical CLI folk as well as @pvinis

Show resolved Hide resolved packages/cli/src/upgrade/upgrade.js Outdated
Show resolved Hide resolved packages/cli/src/upgrade/upgrade.js
@thymikee

This comment was marked as outdated.

Copy link
Collaborator Author

thymikee commented Feb 16, 2019

Made this a non-breaking change by putting new implementation behind --improved flag.

@thymikee thymikee changed the title feat: rewrite upgrade to use rn-diff-purge feat: improve upgrade to use rn-diff-purge behind a flag Feb 16, 2019

@cpojer

This comment has been minimized.

Copy link
Contributor

cpojer commented Feb 17, 2019

This is super awesome, thank you so so much for doing this, it's exactly what I hoped we could do, and you already did it!

Can you just go ahead and make this the default? Let's dump the old way of doing things, it is clearly very problematic for people – what you are doing here cannot be any worse. All we need to make sure is that the right diff is available when we make a new release. As part of that, we may want to consider merging rn-diff-purge into react-native-community. What do you think?

@pvinis

This comment has been minimized.

Copy link
Contributor

pvinis commented Feb 17, 2019

I am currently doing some testing on this, to make sure a couple cases are handled well, but it looks good so far!

@thymikee

This comment has been minimized.

Copy link
Collaborator Author

thymikee commented Feb 17, 2019

Yep, rn-diff-purge should be also fully automated so that once the npm tag is released, a diff comes up few minutes later. Looks like it's tracked in pvinis/rn-diff-purge#10. @pvinis would you mind moving the repo to react-native-community?

Can you just go ahead and make this the default? Let's dump the old way of doing things, it is clearly very problematic for people

Well technically 0.59 (our first consumer) is still in RC, so we could stretch that semver incompatibility, and put the old version behind a flag.

Looks like neither Expo nor Ignite use the upgrade, so we should be fairly safe

@thymikee thymikee requested review from grabbou and Esemesek Feb 17, 2019

@cpojer

This comment has been minimized.

Copy link
Contributor

cpojer commented Feb 17, 2019

Yep definitely recommend swapping the default to this, and keeping the old one around for a little longer. Now is a good time.

@pvinis

This comment has been minimized.

Copy link
Contributor

pvinis commented Feb 17, 2019

I am happy to move this to community, sure, I don't see why not.

For automatic generation, right now I was trying to make a rpi build the diff when an npm version is up, but the npm version is not the easiest. I found a couple of services that email me so I was thinking of hooking this up with a mail check, but I saw npm has hooks but only for a paid org. Any ideas are welcome.

I agree that we might as well make it the default since the previous one seems to be not much used, but it would make sense to keep both for a while, one default and one with a flag.

Also, here are the things I was concerned about:

  • What happens in case of conflicts.
    I saw that the upgrade stops with the files as they are, so the user fixes the conflicts, but the package.json is left untouched. I think we should also update that, so when the conflicts are fixed, the upgrade can be complete.
  • What happens with linking libraries in XCode (like JSC recently) when we have more targets.
    For this, it might be a problem until we move to something like xcodegen? Maybe still this doesn't solve it?
    After testing it, it's as I expected, the change happens only on the first target, and not any more, which makes sense but sucks. Since there is no better way for upgrading the xcodeproj files, I guess it's fine to just keep it like that, and hopefully find a better way soon. Maybe we could show a text that the same changes should happen manually by the user in other targets? (I need to find some time to make a PR with xcodegen and xcconfigs, or even better, instead of diff we use a script for xcode that can loop over targets and do our changes.)
  • What's the git status after the upgrade, which files are staged, unstaged, and why?
    I saw that all files are staged except package.json and yarn.lock. I would say let's stage all or leave all unstaged.
  • Tags
    I saw that if a tag version/x.y.z exists, it's not replaced. If I try to create a tag after an upgrade like version/x.y.z, it's taken by the cli upgrade commits from purge, so that's kinda weird, because then as a user I need to go delete a tag. At least I didn't see problems from having tags with the same name while upgrading.
@pvinis

This comment has been minimized.

Copy link
Contributor

pvinis commented Feb 17, 2019

It would be very useful to have a link to the diff we are doing when it fails.

screen shot 2019-02-18 at 00 21 38

A link to pvinis/rn-diff-purge@version/0.44.0...version/0.47.2 would be useful to see the diff and our conflicts and see what's up.

@pvinis

This comment has been minimized.

Copy link
Contributor

pvinis commented Feb 17, 2019

Actually, after a conflict, the upgrade stops in whatever point it was. I think it might be better to keep it going, so we can see the rest, and solve the conflict.

@pvinis

This comment has been minimized.

Copy link
Contributor

pvinis commented Feb 17, 2019

I tried using it in an actual project and in a test project with a few "bigger" changes like some babel configs, some different xcode setup, typescript etc.
It didn't go well. I was not expecting it to go well, since git patches of a template are not enough to upgrade an actual project, but I wanted to try.

screen shot 2019-02-18 at 00 42 22

I think even with this command, I will still use the manual way to upgrade. We should think about having a migration script instead, that does some work per version. This would need to be manually created.

Let's say there is a new version. We use purge to make the diff. We see the diff, and based on that we have a migration script do the change. If the change is just an edit of a file, like package.json update or babel update etc, we apply it. For android it's mostly simple. For Xcode we can use some library to edit the xcodeproj and do the change (for example linking JSC) but we see that in the tempate (and so in purge) we link the lib to the target. In the migration script we would loop over the targets and link the lib. Then most other changes are small edits in the AppDelegate or App.js etc, and for these again, if we can apply them, good, if not, the script can notify the user, so they do the change.

Maybe what I'm thinking is basically a manual upgrade helper, that applies what it can using a script instead of diff, then applies usin the diff, then guides the user to do the rest of the changes.

@cpojer

This comment has been minimized.

Copy link
Contributor

cpojer commented Feb 17, 2019

@pvinis that's a great idea. I think it would be great if the upgrade command could also print a condensed version of the release notes, like the breaking changes and links with guidance on what to do.

@TheSavior

This comment has been minimized.

Copy link
Contributor

TheSavior commented Feb 18, 2019

Another option is to bail out really early from applying things automatically and to just show the patches. Essentially just an interactive cli walkthrough of the changes in Rn-diff-purge. We can automate that more and more over time when we are confident.

@turnrye

This comment has been minimized.

Copy link
Member

turnrye commented Feb 18, 2019

@cpojer we can definitely do that; with libs like changelog-parser it just works™.

@thymikee

This comment has been minimized.

Copy link
Collaborator Author

thymikee commented Feb 18, 2019

@pvinis thanks for great feedback. This command isn't meant to be a perfect way to upgrade, but it should ease the process as much as possible.

A link to pvinis/rn-diff-purge@version/0.44.0...version/0.47.2 would be useful to see the diff and our conflicts and see what's up.

Added.

What happens in case of conflicts.

We now show the git status output.

What happens with linking libraries in XCode (like JSC recently) when we have more targets.

We fail miserably, but we're able to show the diff and point to the plain diff, so it's still way better than any current workflow.

What's the git status after the upgrade, which files are staged, unstaged, and why?
I saw that all files are staged except package.json and yarn.lock. I would say let's stage all or leave all unstaged.

We now show git status and add package.json and common lockfiles to git stage.

screenshot 2019-02-18 at 11 06 31

Tags
I saw that if a tag version/x.y.z exists, it's not replaced. If I try to create a tag after an upgrade like version/x.y.z, it's taken by the cli upgrade commits from purge, so that's kinda weird, because then as a user I need to go delete a tag. At least I didn't see problems from having tags with the same name while upgrading.

I'm not sure what you mean, could you clarify?

@pvinis

This comment has been minimized.

Copy link
Contributor

pvinis commented Feb 18, 2019

Thanks for addressing the above. I'm looking forward to an Xcode solution in the future :D.

Let me rephrase the tag thing.

Currently, this new upgrade command will basically bring the purge commits and tags in the user's project. If that user has tags in that project, that have the same form as the tags in the purge repo, then the tag will not be in the purge commit, but it will stay in the user's project commit.

On the other hand, if we bring all the commits and tags from purge to the user's project, if that user wants to create a tag with the same form, like if they make a new app release and the tag is version/0.38.0, that tag will be already taken. The user will have to go find the tag and delete it in their repo.

Do you know what I mean, or did I not explain it very well? My concern is basically that we will be messing with tags in a repo that's not ours. But since we remove the remote after, maybe it's not really needed to do anything more. It was just a thought.

@thymikee

This comment has been minimized.

Copy link
Collaborator Author

thymikee commented Feb 18, 2019

Oh no, we must do something about it

@thymikee thymikee force-pushed the feat/upgrade branch from 565a783 to 4e49d8a Feb 18, 2019

@thymikee

This comment has been minimized.

Copy link
Collaborator Author

thymikee commented Feb 18, 2019

@pvinis found that since git 2.13 (Q2 2017) there's git fetch --no-tags that does the trick.

@thymikee thymikee force-pushed the feat/upgrade branch from fe584e1 to df4a154 Feb 18, 2019

@thymikee

This comment has been minimized.

Copy link
Collaborator Author

thymikee commented Feb 18, 2019

Latest iteration on the UI:

screenshot 2019-02-18 at 23 30 00

@thymikee thymikee force-pushed the feat/upgrade branch from 577727c to 8bd3b8b Feb 19, 2019

@pvinis

This comment has been minimized.

Copy link
Contributor

pvinis commented Feb 19, 2019

Awesome. I'll run it again. 👍

@thymikee thymikee changed the title feat: improve upgrade to use rn-diff-purge behind a flag feat: improve upgrade to use rn-diff-purge Feb 19, 2019

@thymikee

This comment has been minimized.

Copy link
Collaborator Author

thymikee commented Feb 22, 2019

@pvinis anything that's holding us back with merging this? I'll be unavailable for the next couple of days so I'd like to get this over the finishing line.

I also have some ideas for further improvements:

  1. fallback to generating the diff locally if diff is not available (the actual procedure is quite straightforward, the only shortcoming is the time to generate it, which involves installing RN twice from scratch)
  2. With 1) implemented, we could easily support custom templates, which would produce way better diffs than from the default one
  3. Transfer the rn-diff-purge to react-native-community (tracked in pvinis/rn-diff-purge#18)
@pvinis

This comment has been minimized.

Copy link
Contributor

pvinis commented Feb 22, 2019

I think we can merge. I like the ideas too :). sorry for the delay.

@thymikee

This comment has been minimized.

Copy link
Collaborator Author

thymikee commented Feb 22, 2019

@Esemesek @grabbou looking for a final look from you 👀

@pvinis

pvinis approved these changes Feb 22, 2019

@cpojer

cpojer approved these changes Feb 22, 2019

Copy link
Contributor

cpojer left a comment

Amazing, I'm so excited about this. Thanks @thymikee and @pvinis. Can you also make sure to improve the upgrading guide on the website? :)

@thymikee thymikee merged commit 06036f5 into master Feb 22, 2019

3 checks passed

ci/circleci: install-dependencies Your tests passed on CircleCI!
Details
ci/circleci: lint-and-flow Your tests passed on CircleCI!
Details
ci/circleci: tests Your tests passed on CircleCI!
Details

@thymikee thymikee deleted the feat/upgrade branch Feb 22, 2019

@thymikee

This comment has been minimized.

Copy link
Collaborator Author

thymikee commented Feb 22, 2019

@pvinis mind taking care of the docs?

@pvinis

This comment has been minimized.

Copy link
Contributor

pvinis commented Feb 22, 2019

You took care of the docs before I saw this comment, and you did a great job too! :)

@darknblack

This comment has been minimized.

Copy link

darknblack commented Feb 24, 2019

Thank you and great work =)

@vomchik

This comment has been minimized.

Copy link

vomchik commented Feb 25, 2019

I love it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.