0.57.4 Discussion

[kelset]

Conversation on this thread are limited to 0.57.4 release's major issues and backport (cherry-pick) requests from commits that are already on master - so that we can then produce a 0.57.5 release in the near future.

An example of a good such request is a bug fix for a serious issue that has been merged into master but did not make the 0.57.4 cut.

In other words, if you cannot point to a particular commit on master, then your request likely belongs as a new issue in http://github.com/facebook/react-native/issues.

[kelset]

Catching up from #48:

• the React sync for 16.6.0 will land in the near future, but I did a couple tests and you should be able to use it without issues* already (will require this too facebook/react-native@8325e09)
• The XCode10 related fix commit (facebook/react-native@b44c5ae) didn't end up in 0.57.4 because its cherry-picking was super complex. We'll give it another try this for release (in the meantime we workaround has been written directly in the changelog to help)
• other commits that have been requested since the list was finalized (so we'll prob attempt to cherry pick this release)
  • facebook/react-native@b002df9
  • facebook/react-native@09c78fe

---

Edit:
* seems like in some scenarios moving to 16.6.0 is actually not behaving properly, so be careful

[mikemorris]

• Fix incorrect merged asset path with flavor for Android Gralde Plugin 3.2 Fix incorrect merged asset path with flavor for Android Gradle Plugin 3.2. facebook/react-native#21782

(I'm aware it hasn't been merged yet, but it's a small yet critical fix and been successfully tested by a few users now.)

[ikesyo]

I nominate

• Quote "$NODE_BINARY" in react-native-xcode.sh facebook/react-native@7d4e94e

(From #48 (comment))

[a-tarasyuk]

Is there any timeline when will be published version 0.57.5?

[dryganets]

It doesn't look like it was backported.
You could use this workaround for now:

ReadableNativeArray.setUseNativeAccessor(true);
ReadableNativeMap.setUseNativeAccessor(true);

[rajivshah3]

Is it possible to get facebook/react-native@7a914fc included in this release? This commit will fix our Jest tests and allow us to upgrade to v0.57.x

[kelset]

I'm still waiting for the React sync commit to land, so until that commit is on master 0.57.5 won't be released.

@mikemorris the PR is currently being imported, hopefully it will land on master soon.

@dryganets I'm not sure I understand what you are referring to 😅

[ikesyo]

the PR is currently being imported, hopefully it will land on master soon.

That was successfully landed. 🎉 So let me nominate the commit:

• Fix incorrect merged asset path with flavor for Android Gradle Plugin 3.2. facebook/react-native@e90319e

[radeno]

Till it will be synced to React 16.6 my two cents to PRs:

Updated

• All commits before my comment
• Flow update: 1. facebook/react-native@11552a7 2.facebook/react-native@adc8a33
• Update Jest: 1. facebook/react-native@1b4fd64 2. facebook/react-native@66aba09
• Metro update: 1. facebook/react-native@f867db3 2. facebook/react-native@8888295 3. facebook/react-native@31bb551
• Update fbjs-scripts facebook/react-native@cdbf719
• Fix gradle repo priority facebook/react-native@2a349f8
• Fix relayout of inline view facebook/react-native@798517a
• Fix IllegalStateException when dismissing DialogManager in Android facebook/react-native@38e01a2

[sampettersson]

Would be nice if we could get facebook/react-native#21179 into 0.57.5, it solves issue facebook/react-native#18997.

[dryganets]

@kelset
I'm talking about commit for this issue:
facebook/react-native#18292

[brunolemos]

Suggestion for 0.57.5:

• Remove global babelHelpers and regenerator facebook/react-native@458d56c (Remove global babelHelpers and regenerator facebook/react-native#21283) to hopefully fix Module @babel/runtime/helpers/interopRequireDefault does not exist in the Haste module map facebook/react-native#21310

[kelset]

@brunolemos that commit is already in 0.57.x since .2 -> facebook/react-native@5627616

Small update: the sync commit landed during the weekend but it caused some crashes so it was reverted (This is one of the benefits of Facebook running on master 😅).

[grabbou]

This commit facebook/react-native@506f920 should be cherry-picked to resolve issues caused by a breaking change that recently shipped. This is to fix multiple crashes that affect RNNavigation by Wix users.

[ikesyo]

@grabbou Are you saying about facebook/react-native@b002df9? That is already nominated in #48 and #54 (comment).

[fungilation]

@grabbou, this alongside syncing with React alpha. Can RN generally not cherrypick big changes that's potentially breaking on minor releases (0.57.x)? I'm not seeing enough stability on minor releases that's now used for integrating new features. Shouldn't more major changes wait for cherrypicking on the next major release?

My 2c and whether this should be a discussion on a new issue, if there's much to discuss.

[kelset]

Can RN generally not cherrypick big changes that's potentially breaking on minor releases (0.57.x)?

It's about tradeoffs, since 0.58 is going to have some major breaking changes. We'll discuss more about this during the next core meeting happening later this week.

In the meantime, since I haven't got any update on the state of the React sync from the FB team we'll probably test a new 0.57.5 without it because I feel that this release is long overdue (there are already many commits I want to rollout, like the one to restore UIImplementationProvider).

[radeno]

About cherry-picking something significant like increasing React version is double-edged.

On one side when is everything OK updating packages should be fine, because if is package based on semantic versioning then updating minor version should be without any disasters. We are using 0.57.4 without any problems. But i agree we are not all.

On the other hand we don't really know how are two software products coupled together in 100%. They could be coupled with non-public API, internal hooks or something else than documented public APIs, like we seen with change schedule to scheduler in React 16.5.3 and current regression on 16.6.0. Both steps forward breaks another software (React Native). It was not supposed to happen but does.

I am usually happy to touch "newest" technology as quick as possible, anyway stability of (any) product is major priority. Even more if it is just some minor or patch release.

@kelset is RN 0.57.4 compatible with React 16.5.2 ? I don't test it yet. If so, we can add note to changelog, that if user have any issues with 16.6 alpha it can be safe downgraded.
I agree to release 0.57.5 as soon as possible. Some important cherry picks waits. You will see more after discussion with core team.

[kelset]

is RN 0.57.4 compatible with React 16.5.2 ?

AFAIK no, since 0.57.2 you need to use the 16.6 alpha.

That said, I tried to cherry pick & test this list of commit (order of commit):

• Incorrect ref API usage fixed.
• Fix InputAccessoryView safe area when not attached to a TextInput
• Remove previously scheduled NetInfo callbacks if they haven't fired
• Remove unused parameters in ReactInstanceManager methods
• Update size of Root ShadowNode when RootView changes its size
• Add systrace to UIManagerModule.createConstants
• Fix setupReactContext systrace
• Remove class loads for CoreModulesPackage
• Initialize getConstantsForViewManagers eagerly
• Re-introduce UIImplementationProvider
• RN crash when processing Exception with null message
• Fix flow in InitializeCore
• Fix polyfilling of regeneratorRuntime to avoid setting it to undefined in some situations
• Don't crash when ReactRootView's id is already set
• Remove hard crash when root view is reused
• fix android ci
• Quote "$NODE_BINARY" in react-native-xcode.sh
• RN: Reduce ForwardRef(View) Noise in Systrace
• Fix View/Text displayName
• Bump metro@0.49.0
• Bump fbjs-scripts to ^1.0.0
• Wrap measureLayoutRelativeToContainingList in try-catch to mitigate crash
• Fix incorrect merged asset path with flavor for Android Gradle Plugin 3.2
• gradle repo priority
• Fix relayout of inline views
• Fix ReactRootView mount/unmount race condition
• Remove undefined value on init cli command
• bump buck to 2018.10.29.01. fixes Android CI
• Added lock around RN module initialization to fix crash
• Added locking around RN bridge cxx module registry to avoid crash
• Performance improvement for loading cached images on iOS
• Increase cache and file size limits

But while testing I experienced an hard crash on Android, and after a lot of investigation I understood that this commit Remove class loads for CoreModulesPackage is the one introducing it. I have already reached out to the FB developer to help me understanding what am I missing / where is the fix, and hopefully I'll have something fully working locally.

On top of this then I'll manually apply the Xcode 10 third party commit - since it's the only way to avoid a billion merge conflicts on the Xcode proj file (@radko93 has been helping me with this).

In any case, this new release won't land before the core meeting on Thursday, but I wanted to be transparent so that maybe someone can help understanding what causes that Android error.

[fungilation]

Looks like a great list of mostly bug fixes. Thanks @kelset.

At the FB meeting, any possibility to bring up accelerating a sync with React 16.6.x release? I want to upgrade my RN use but I personally still don't want to with a React alpha dependency.

[hramos]

@fungilation it's been worked on. These syncs are not quite straightforward to perform, and this time around we're getting more people from across Facebook's JavaScript tooling teams to jump in to see if we can streamline the process for future releases.

In this particular case, we landed the 16.6 sync last week (facebook/react-native@8b275a8), but were forced to revert it due to some critical bugs we encountered (facebook/react-native@6448f4e). We're working hard to move past these issues and land a 16.6 sync in master again.

[mmccartney]

@kelset Would it help if I squash my changes under
facebook/react-native#21458 into a single commit for you?

I'm sure I can handle the git operations but not sure where best to place it for cherry-pick. I could put it in my personal fork if that helps...

[kelset]

Update on my side: I've talked with the FB team about the Android crash and by removing some commits that I have cherry picked I managed to fix the issue I posted above. But then I stumbled again on it (= Android crash during test phase) when trying to upgrade to Metro 0.49 (adding the jest 24 alpha commits don't seem to affect the outcome, but I've learned is actually required for Metro to work properly) 😰

The list of commits I managed to pick without issues is:

• Incorrect ref API usage fixed.
• Fix InputAccessoryView safe area when not attached to a TextInput
• Remove previously scheduled NetInfo callbacks if they haven't fired
• Remove unused parameters in ReactInstanceManager methods
• Re-introduce UIImplementationProvider
• RN crash when processing Exception with null message
• Fix flow in InitializeCore
• Fix polyfilling of regeneratorRuntime to avoid setting it to undefined in some situations
• fix android ci
• Quote "$NODE_BINARY" in react-native-xcode.sh
• RN: Reduce ForwardRef(View) Noise in Systrace
• Fix View/Text displayName

And this is the "backlog" I want to have for the release too:

• jest: upgrade to 24.0.0-alpha.4 plus Bump metro@0.49.0 (current blocker with the Android crash)
• Bump fbjs-scripts to ^1.0.0
• Wrap measureLayoutRelativeToContainingList in try-catch to mitigate crash
• Fix incorrect merged asset path with flavor for Android Gradle Plugin 3.2
• gradle repo priority
• Fix relayout of inline views
• Fix ReactRootView mount/unmount race condition
• Remove undefined value on init cli command
• bump buck to 2018.10.29.01. fixes Android CI
• Performance improvement for loading cached images on iOS
• Increase cache and file size limits

That said, if anyone want to take a stab at understanding what's wrong, you can (if you are on a mac):

• checkout the main repo to 0.57-stable
• use git cherry-pick [hash of the commit] to cherry pick the commits in order
• run ./scripts/test-manual-e2e.sh (make sure you have the NDK17 and an android device or simulator open before running)

@mmccartney thanks for wanting to help, I think that the best thing would be, once we've managed to cherry pick the whole list without issues, for you to try and replicate the fix on top of 0.57-stable once we pushed these commits on top of it - so first need to unlock the issue above sadly. (btw if you are on twitter pls DM me there so that we can coordinate more easily)

[kelset]

Hey everyone - quick update: I've attempted once more to cherry pick the commits and test locally, and by removing the ones bumping jest, metro and fbjs-script I was able to have everything ✅ locally. So I proceeded to add on top of that the React sync, so that we can move away from the 16.6 alpha into 16.6.1 (which, from my understanding, should also enable hooks - but pls double check) - I've retested everything locally and still ✅.

I've now pushed this list of commits to the remote branch for the CI tests.

A couple of other notes:

• @mmccartney kindly offered to do a PR against 0.57-stable with the Xcode 10 fix, so once that's up, I'll merge (or replicate or something) then I'll retest locally, and then I'll proceed to bump & release 0.57.5
• I'll write up in more details the core meeting in the discussion repo, but one of the things we agreed on is that once this patch lands we won't change version of React or any other 'major' package for 0.57.x (also because 0.58 rc0 should be released soon). Another is that we will be more conservative on accepting cherry picks requests. Anyway I'll link the notes once they are up.

---

EDIT: ok, CI is being a bit annoying, something wrong with Flow and Metro. You can check the errors here and to repro simply checkout to 0.57-stable, and run yarn flow check. I feel it's something that should be easy to fix, but I won't be able to investigate more 'til Monday and any help would be really appreciated :)

[fungilation]

Want to confirm, that React 16.6.1 with sync will ship in RN 0.57.5? Hurray if so.

As for being more conservative on accepting cherry picks, I assume that's for the minor version releases? Wouldn't want to slow down progress in the main releases with rc's.

[hramos]

I understand it would be for both. From a maintainer's point of view, it's the same amount of work to land a cherry-pick in 0.57.5 as it would be on 0.58.0-rc.1 once the RC is cut.

I'd venture to say we should be even more conservative about commits that are cherry picked into the RC over a patch version, since the goal of the RC is to ensure the next release is stable: cherry-picks into a RC should strictly fix regressions introduced by the RC cut itself. Take for a example a change that made it into the RC that depends on a newer version of Metro, but the Metro bump commit missed the RC cut - it would be reasonable to cherry-pick the Metro bump commit. On the other hand, a fix for a long standing issue that landed on master after the RC cut can wait for the next patch release once RC hits stable.

This is something that is still in discussion, and keep in mind that maintainers can be convinced otherwise by simply putting in the work. If you strongly believe a commit can't wait for the release, sending a PR to the stable branch with the commit applied cleanly and all tests passing will go a long way towards making your case. Part of the motivation behind this discussion is the fact people often asks for commits to be cherry-picked without verifying if there are other commits that need to be pulled in as well in order to keep tests green.

[rajivshah3]

CI is being a bit annoying, something wrong with Flow and Metro

There's a few issues happening:

Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ node_modules/metro/src/HmrServer.js.flow:99:37

Cannot call formatBundlingError with new GraphNotFoundError(...) bound to error
because undefined [1] is incompatible with number [2] in property lineNumber.

Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ node_modules/metro/src/HmrServer.js.flow:110:37

Cannot call formatBundlingError with new RevisionNotFoundError(...) bound to
error because undefined [1] is incompatible with number [2] in property
lineNumber.

Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ node_modules/metro/src/HmrServer.js.flow:174:13

Cannot call formatBundlingError with new RevisionNotFoundError(...) bound to
error because undefined [1] is incompatible with number [2] in property
lineNumber.

I'm not sure what these mean, but upgrading flow-bin to 0.86 seems to fix them. The downside is that the upgrade reports over 40 flow errors (the current one only reports 4).

Edit: 0.83 seems to get rid of them as well, and only introduces a few more errors compared to 0.78

Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ node_modules/metro/src/index.js.flow:130:51

Cannot resolve module metro-visualizer.

I think this was fixed in facebook/metro#278 but later reverted in facebook/metro@aba22ad (I guess a new version of flow was supposed to fix it?). Nonetheless, upgrading flow-bin still doesn't fix that issue. I'll keep looking into this, but I thought to post it here in case anyone else is more familiar with flow

[mmccartney]

facebook/react-native#22230 has my work from facebook/react-native#21458 done as a single commit (for easy picking) and against the 0.57-stable branch. Have a great weekend everyone!

[kelset]

Good Monday everyone - and thanks for keeping the conversation moving:

• I second everything Hector said, and again it's an open discussion (the notes of the meeting will land soon, the PR is already up)
• Luckily I have enough permissions in the main repo to be able to merge @mmccartney's PR (thank you so much again!), I'll test it locally 👏
• Thanks @rajivshah3 for your investigation, I can understand that potentially upgrading Flow fixes the issues but that's probably the least favourable way (it would force all the people on 0.57.4 to upgrade their flow too AFAIK) - I'll check if upgrading Metro is a viable solution, and I'll report back. This because AFAIK react-native init will actually pull the latest Metro anyway, and that means needing Jest 24 because of a breaking change on Metro 0.48.3 (sigh). Anyway I'll keep you all updated 🤞

[kelset]

Update:

• merged the PR from @mmccartney
• I managed to fix flow without having to change Metro or Jest or Flow version, but simply tweaking around the Flow config and adding a couple of FlowFixMe in the files (which, weirdly enough, already partially had similar FlowFixes 🤷‍♂️)
• I ran more tests locally and they seem to suggest that the Xcode 10 PR is not fixing completely the issues related to third party libraries, but I can't tell for sure if it's an issue only on my machine, since the CI is now green.

This means that technically I could proceed with the bump commit and release 0.57.5.

Before that, though, I'd like to ask you to test* it via changing your dependencies in the package.json to

    "react": "16.6.1",
    "react-native": "facebook/react-native#0.57-stable",
    "react-test-renderer": "16.6.1"

My test with it forced me to re-apply the workaround fix for Xcode 10 to build iOS properly, so please let me know if it was just something with the machine I used for testing - because I'd really prefer not to create new or more complex issues for those who will upgrade to this version.

Based on the feedback I'll get in the next 24hrs we'll decide with the core if we can proceed with the release.

---

• iOS tests only, gradle doesn't work well with RN pulled directly

[ptomasroos]

I will try internally in our circle ci @kelset but here's something public with detox tests https://travis-ci.com/ptomasroos/react-native-idfa/builds/91078892

[iainbeeston]

Works for me locally using Xcode 10.1, without running the workaround fix for 3rd party dependencies.

[radeno]

Works for me.
Xcode 10.1, React 16.5.1, even with Metro 0.49

[ptomasroos]

All green in our internal ci + test environment, both for android + ios (we're a cocoapod user since 2y ago, just for the record that nothing broke in the podspec)

[kelset]

Thanks everyone for the feedback, I'll proceed with the release later today.

@a-tarasyuk as I mentioned in my comment, Android has issues when RN is pulled from GitHub ;)

[ptomasroos]

We compile everything locally (because of possibility to cherry pick ourselfs) and that shows no sign of issues on android FYI @kelset

[kelset]

Thanks everyone for helping out with the discussion, 0.57.5 just landed on npm.

I'll close this issue in favour of a new one to discuss the next patch release.