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

Upgrade to React Native 0.56 & 0.57 #2789

Merged
merged 7 commits into from Nov 1, 2018
Merged

Upgrade to React Native 0.56 & 0.57 #2789

merged 7 commits into from Nov 1, 2018

Conversation

borisyankov
Copy link
Contributor

@borisyankov borisyankov commented Jul 12, 2018

Upgrade dependencies, config and build files to RN 0.56

Previous upgrades were done manually by updating dependencies and
.flowconfig

This commit contains a full diff with previous versions.

What was done is:

  • run react-native-git-upgrade
  • resolve merge conflicts
  • update dependencies that were needed but missed by the script

Note:

@borisyankov
Copy link
Contributor Author

This is a definitive fix for the Jest issue:
vovkasm/react-native@cdc3e8c

@zulipbot
Copy link
Member

Heads up @borisyankov, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

@gnprice
Copy link
Member

gnprice commented Aug 1, 2018

Cool!

It looks like there are two cleanups here that are ready to go, then a WIP commit, and there are still some other fixes to be made.

I've just gone and merged those two commits that are ready. In general that's the way I'd like to approach a migration like this: whatever changes need to happen, where possible we'll go ahead and make them (as nice cleanly separated commits, just like these) before actually cutting over for the migration.

As you produce more, if you go ahead and rebase -i them to the start of the branch, that will help signal that those commits are ready.

@borisyankov
Copy link
Contributor Author

I actually have 5-6 more of these fixes locally, will clean them up and push to review (they should all be independent of the upgrade to RN 0.56 but a requirement to not get the errors once upgraded)

@armaanahluwalia
Copy link
Collaborator

@gnprice @borisyankov I'm actually interested in getting this PR merged so I can move forward with trying to get #2887 merged in!

Wondering if this would be possible soon?

@borisyankov
Copy link
Contributor Author

@armaanahluwalia apart from few Flow issues that need fixing, this PR is ready to go.

Give it a try and give us feedback if you are having any issues. If it works for you, that is one more assurance that will let us merge faster.

We are also looking to merge this one #2955

@armaanahluwalia
Copy link
Collaborator

Okay I’ll do that and get back to you. Is there any reference document for manually testing the entire application? Something that might guide someone looking to test all the major features?

@borisyankov
Copy link
Contributor Author

Not really. Manual testing almost by definition is ad-hoc.

@gnprice
Copy link
Member

gnprice commented Sep 16, 2018

@borisyankov Do you believe some of these commits are ready? (It sounds like yes.) It'd help a lot if you would rebase -i them to the front of the branch, so that there's a specific series of changes you've tested and are recommending to merge.

@gnprice
Copy link
Member

gnprice commented Oct 22, 2018

Ping. As described in my comment above, there are several items that are on your plate. Let's prioritize getting this finished.

@borisyankov
Copy link
Contributor Author

Not sure if clear from any comment but:

1669c68 Fix Xcode project settings warnings
was updated with both release and debug warnings

@gnprice
Copy link
Member

gnprice commented Oct 23, 2018

OK, to be specific at risk of repeating myself: items on your plate, other than the ones you've split out as separate PRs, include

Would you please address these?

@borisyankov
Copy link
Contributor Author

I am going through these, in the meanwhile, I went through the commits that update yarn.lock and rerun yarn with the latest version to ensure integrity hashes are included.

@borisyankov
Copy link
Contributor Author

I worked on this:

The Jest commit. I had some comments and questions above: #2789 (comment)

I remember reading it before, but now I went through in detail on everything that was discussed and issues that were linked to.

I spent few hours on trying to make a babel.config.js and it isn't working. It seems it can work with a lot more messing with configuration and plugins etc. but I don't think it is worth it.

@borisyankov
Copy link
Contributor Author

A couple of hours more on this issue, suggest that several people found the solution:

"transform": {
  "^.+\\.js$": "<rootDir>/node_modules/react-native/jest/preprocessor.js"
},

works fine for them.

@borisyankov
Copy link
Contributor Author

Squashed the two XCode commits and updated the commit message.
Also rebased onto master.

gnprice and others added 7 commits October 31, 2018 16:33
It's less over-broad than ours in one very nice way: for every
legitimate style prop, it specifies the right type for its values
(right down to the allowed set of values for things like
`flexDirection`), and will cause the type-checker to complain on an
ill-typed value.
Without this change, after the upcoming upgrades Flow gives error
messages here.  If I understand the error messages correctly (though
I'm not confident I do), the point is that these particular components
insist on their props being read-only -- and while ViewStyleProp and
friends make that guarantee, DangerouslyImpreciseStyleProp doesn't.

These will be resolved properly when we eventually go about and narrow
down uses of DangerouslyImpreciseStyleProp to the more specific types.
Until then, just leave these fixmes.
Holding back from v0.57.2 because of issues there.

Skipping v0.56 because `jest` fails with a Babel config issue (with
or without the Babel config change in this commit), and at this point
v0.56 is old enough it's not worth trying to sort that out.

Both upgrade tools (react-native-git-upgrade and `react-native
upgrade`) failed to run, so instead this was a manual but thorough
old-school upgrade.  Not a problem, since we did that already for all
but the last upgrade.

Most of the changes required for the upgrade have been merged in a
variety of commits over the past few weeks; see zulip#2789.  This commit
 * upgrades `react-native` and closely related dependencies;
 * updates the Flow config file to match RN upstream's template; and
 * adjusts the Babel config to work now that most things use Babel 7:
   move the preset back to our specific babelrc, and re-enable babelrcs.

Fixes zulip#2788.
Typically we'd do this in tandem with the dependency upgrade, but
there's a lot of noise here and there are several substantive changes
in the parent commit; and, happily, the type-checker already passes at
the parent without this update.
Remove the $FlowFixMe comments as Flow no longer rises:
`Props has no `narrow` property`
The props we use to set the colors of the Switch component,
`onTintColor` and `tintColor`, are now deprecated.

Update to their replacement, the new `trackColor` property, setting
the exact same values to preserve the actual style unchanged.
@gnprice gnprice merged commit 95f41fc into zulip:master Nov 1, 2018
Next automation moved this from In Progress to Done (unreleased) Nov 1, 2018
@gnprice
Copy link
Member

gnprice commented Nov 1, 2018

And merged! 🎊 Thanks @borisyankov for all your work on this 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Next
  
Done (latest first)
Development

Successfully merging this pull request may close these issues.

None yet

4 participants