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

0.57.3 Discussion #48

Closed
grabbou opened this Issue Oct 11, 2018 · 46 comments

Comments

Projects
None yet
@grabbou
Contributor

grabbou commented Oct 11, 2018

Conversation on this thread are limited to 0.57.3 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.4 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.3 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.

@TopGuo

This comment was marked as off-topic.

TopGuo commented Oct 12, 2018

If used for company production development, which react native version is more stable?

@ahce

This comment has been minimized.

ahce commented Oct 12, 2018

@grabbou when release 0.57.3 in npm?

@kelset

This comment has been minimized.

Collaborator

kelset commented Oct 12, 2018

Hey everyone, there was an issue with the release, I'm looking into it today

Ok, release is out now on npm.

One thing: when you upgrade to this version you NEED to upgrade react and react-test-renderer to version "16.6.0-alpha.8af6728". It's safe, I've tested 0.57.3 in multiple apps without issues. (if you prefer to not, you can wait for future 0.57.x releases, 16.6.0 will be most likely released in a couple weeks at ReactConf).

@grabbou

This comment has been minimized.

Contributor

grabbou commented Oct 13, 2018

We should cherry-pick facebook/react-native@b44c5ae to improve Xcode 10 support and make sure it's all good at the time of 0.58 release.

@peat-psuwit

This comment has been minimized.

peat-psuwit commented Oct 13, 2018

Please cherry-pick Update link command for Android project. This commit fixes Android project auto-linking to be compatible with already-released Gradle plugin upgrade.

@matt-block

This comment has been minimized.

matt-block commented Oct 13, 2018

@peat-psuwit that update did not cover unlinking, so I think it should only be cherry-picked together with the unlink PR when it lands.

@peat-psuwit

This comment has been minimized.

peat-psuwit commented Oct 13, 2018

@matt-block I agree with you. Thank you for pointing it out!

@radeno

This comment has been minimized.

radeno commented Oct 13, 2018

@grabbou maybe we should have one up-to-date comment with approved cherry-pick commits. Like in forums where first topic comment is always updated (eg instructions to install something for current app version). Something like @kelset do in previous version #46 (comment) What do you think? It allows us to update changelog continuously.

Updated With @kelset help

Anyway some patch commits:

@kelset

This comment has been minimized.

Collaborator

kelset commented Oct 15, 2018

@radeno:

  • the NDK one is already in 0.57.3
  • You link a couple of Fabric related commits, I don't understand why we should cherry-pick those - is there any particular reason? 🤔
  • Unless there is a strong need for new Flow I don't think we should bump it in a patch-level release
@radeno

This comment has been minimized.

radeno commented Oct 15, 2018

@kelset
You are right about all points. My post is updated. So slimmer.

@yinhangfeng

This comment has been minimized.

yinhangfeng commented Oct 15, 2018

0.57.3 android have a accessibility role crash issue facebook/react-native#21754
it happens because incomplete cherry-pick from master facebook/react-native#21754 (comment)

@kelset

This comment has been minimized.

Collaborator

kelset commented Oct 15, 2018

Hey @yinhangfeng thanks for pointing that out - if I understand correctly, we need to cherry pick these two

to fix it right?

( cc @peat-psuwit)

@peat-psuwit

This comment has been minimized.

peat-psuwit commented Oct 15, 2018

The actual fix is facebook/react-native@1f96ff6, but do cherry-pick facebook/react-native@139559f first to avoid conflict. (Also, revert facebook/react-native@1592a8d)

@janhesters

This comment was marked as off-topic.

janhesters commented Oct 16, 2018

It would be great to fix testing either in the next 0.57.x release, or for 0.58. The fact that jest.mock() doesn't work and that there is no working workaround is terrible for apps using RN in production.

If there aren't any commits to cherry pick, maybe a description for the changelog how to fix this would be awesome.

@kelset

This comment has been minimized.

Collaborator

kelset commented Oct 16, 2018

@janhesters as mentioned way too many times across these issues, please keep the conversation on commits already on master.

That said, on testing, there are things you could do via PRs, like this to help with it. And also you could submit PRs to the changelog: there is a "known issue" section precisely for those kind of scenarios.

@janhesters

This comment has been minimized.

janhesters commented Oct 16, 2018

@kelset I'm sorry, my mistake.

I think I'm not good enough create a PR and fix this issue (I wouldn't even know where to start). But I did a PR for the changelog. Thank you! 😊

@radeno

This comment has been minimized.

radeno commented Oct 17, 2018

Updated.
Another posible fixes to cherry-pick:

@youngjuning

This comment was marked as off-topic.

youngjuning commented Oct 17, 2018

// @name Make ReactNative Great Again
// @description Allows you to enable support for JSX files, and `.mjs` files which is the new node standard
// @source http://www.fallingcanbedeadly.com/posts/enabling-react-native-jsx-extension
// @issues https://github.com/airbnb/javascript/issues/982
// @issues https://github.com/facebook/metro/issues/248
// @note One caveat, The `index.js` file in the root of your project has to be `.js`.
module.exports = {
  getSourceExts: () => ['jsx', 'mjs'],
}

is not work on 0.57.3

@mikemorris

This comment has been minimized.

mikemorris commented Oct 17, 2018

@janicduplessis

This comment has been minimized.

janicduplessis commented Oct 19, 2018

facebook/react-native@b0d68c0 fixes a crash that happens with react-native-svg (and possibly others) on Android.

@kelset

This comment has been minimized.

Collaborator

kelset commented Oct 19, 2018

Thanks everyone, I'll try to review & finalise the list of cherry-picks today & trigger the release this weekend (has been sick for the past two days 🤢).

@Titozzz

This comment has been minimized.

Contributor

Titozzz commented Oct 19, 2018

Heya, facebook/react-native@fd744dd might be interesting !

I know that some folks at archriss/react-native-snap-carousel#203 would enjoy it 😄

@eldh

This comment has been minimized.

eldh commented Oct 19, 2018

Would love to see facebook/react-native@fa6035b get into next release. It should fix testing with Appium (see facebook/react-native#21238)

@kelset

This comment has been minimized.

Collaborator

kelset commented Oct 19, 2018

@eldh

This comment has been minimized.

eldh commented Oct 19, 2018

Awesome, don't mind me then 😄

@kelset

This comment has been minimized.

Collaborator

kelset commented Oct 19, 2018

Hey everyone - based on the list of commits listed above (plus a few more) I attempted a cherry-pick test locally to prep 0.57.4. It wasn't either easy or funny, so I didn't end up pushing to the remote branch what I've done.

Here's the breakdown, so that either myself or @hramos or @grabbou can pick it up quickly again - and we can discuss if we want to proceed with a subset of these commits or something.

I've managed to cherry-pick and test with everything these commits (ordered in the same order I cherry picked them):

Aside from those (and here starts the complex stuff) the React sync for revisions d836010...4773fdf adds a warning about context, more precisely

Deprecate context object as a consumer and add a warning message

And I'm pretty confident we don't want to spam it to everyone.

THEN, I also wanted to cherry pick those:

But they forced me to get into the spiral of these commits

at least, and then everything was a mess of conflicts so I gave up of getting them in.

Lastly, other 3 I wanted to cherry pick but had conflicts not simple to fix

I know the last one should help with Xcode 10 but it's a mess to cherry pick 😞


So, after all this, I believe that we should proceed with a 0.57.4 with the first list I presented.
LMK

@matthargett

This comment has been minimized.

matthargett commented Oct 19, 2018

It looks like the noisiness of that React Context warning was lessened in a later commit:
facebook/react@9ea4bc6

But if we don't have a way forward with React 16.6 stable, then I would propose reverting the React sync cherry picks from before and staying on 16.5 for the rest of the 0.57 releases. Taking an LTS onto a React alpha make me queasy, but leaving it on a React alpha is not great and gives me flashbacks to React Native 0.43.

@fungilation

This comment has been minimized.

fungilation commented Oct 19, 2018

Thanks for all the cherry picks @kelset! Hope you are recovering.

On React alpha, I for one don't want to upgrade to RN 0.57.3 because of it. As a matter of principle, RN having no alpha or beta labeling shouldn't ever depend on alpha/beta versions of critical components. Why the rush? Let React 16.6 stabilize before cherry picking all the sync commits associated.

Looking at https://github.com/facebook/react/blob/master/CHANGELOG.md, I don't see anything important between the minor versions (16.5.0 -> 16.5.2). I'm perfectly fine on React 16.5.0.

@a-tarasyuk

This comment has been minimized.

a-tarasyuk commented Oct 19, 2018

@kelset Thanks for the detailed update, and for your effort to prepare this release. I hope that Fix Xcode 10 errors relating to third-party will be merged to 0.57.4 ;) - it contains many changes, however, it can resolve annoying issues with the new Xcode.

@kelset

This comment has been minimized.

Collaborator

kelset commented Oct 20, 2018

Thanks for everyone's feedback.

Related to the React alpha issue: I understand, but we are talking about an alpha of a minor version update, which is less likely to create issues (I haven't found any during my time with 0.57.3 in our prod app nor during my testing).

Since it's the weekend and my test yesterday wasn't quick nor easy, here's my proposal for now: this coming week there's ReactConf (October 25 & 26). So we'll soon have a clearer idea of when 16.6.0 will be out: I expect (but have no knowledge) that it will be out during the event, but if that's not the case (ex. they decide to release it in a month) we'll reverse the React sync.

This will mean delay the release of 0.57.4 of (at least) another week.

Or, we do a 0.57.4 with the "already tested" subset of commits I listed above and then for 0.57.5 we consider the 16.5.0 rollback (which means that people can't use React 16.5.2 btw).

LMK

@fungilation

This comment has been minimized.

fungilation commented Oct 20, 2018

Since what's done is done and I've said my piece regarding syncing with alpha, I'm for waiting another week if React 16.6.0 may likely be out then. Least work option, unless it's not out then.

@a-tarasyuk

This comment has been minimized.

a-tarasyuk commented Oct 20, 2018

In my opinion better to release RN which will work correctly with stable React version. (16.5.2 or 16.6.0). Waiting one week to have a stable RN version which will work with stable React (does not matter 16.5.* or 16.6.*) version is absolutely OK.

@fungilation

This comment has been minimized.

fungilation commented Oct 20, 2018

You/I have RN 0.57.2 on React 16.5.0.

@ikesyo

This comment has been minimized.

ikesyo commented Oct 21, 2018

I want the following to be cherry-picked

@ahce

This comment has been minimized.

ahce commented Oct 21, 2018

Is possible RN 0.57.4 with React 16.5.0?

@grabbou

This comment has been minimized.

Contributor

grabbou commented Oct 22, 2018

Personally, I don't mind using an alpha version for a while as React team is wrapping up the stable release. However, I am getting constant impression that the community is having a different opinion in that matter and prefers staying on a stable bandwagon, even if that means missing out on small optimisation and improvements for a while.

That said, I think we shouldn't publish a stable release of React Native that depends on an alpha version of React (even if that's an unstable release of a minor bump - there's always policies at companies). Let's make it a rule from now on.

Now, since we have already released React Native 0.57.3 with an alpha dependency, I would say we continue our cherry-picking process the regular way. That said, I second what @kelset already said. Let's push 0.57.4 out for those who might need these changes and wait for a week to decide whether to rollback or sync with latest.

PS. If someone wants an alpha version of React with a stable React Native release, we can always release 0.57.3-alpha to match React dependency for the time being.

@gwmccull

This comment has been minimized.

gwmccull commented Oct 22, 2018

@grabbou I'm in 100% agreement about not using an alpha of React. I had to do the upgrade to the version of React Native that used React 16 alpha (might have been 44?). It was terrible. Enzyme didn't support the alpha version of React and a lot of other libraries were broken

I don't think any new feature of React is worth that pain

@kelset

This comment has been minimized.

Collaborator

kelset commented Oct 23, 2018

Thanks everyone for contributing to the discussion - we'll keep it in mind for future releases (we are also planning a core meeting precisely on these subjects in early November).

That said, I've spoken with the core team and the FB internal team and since "A non alpha version of React will be landing soon" I won't revert the commit (but won't cherry pick the new one I mentioned earlier) - and proceed to do again a test 0.57.4 local & eventual release.

If you prefer to not use 16.6.alpha you will have to wait a big longer - but, again, in terms of "real usage" I haven't had any problem so far with it (our main app in prod has 0.57.3, been out since last weekend, no issues).

@mikemorris

This comment has been minimized.

mikemorris commented Oct 23, 2018

@ikesyo

This comment has been minimized.

ikesyo commented Oct 23, 2018

@mikemorris That is already in 0.57.2 facebook/react-native@a4fed6e.

@fungilation

This comment has been minimized.

fungilation commented Oct 24, 2018

Well, I see React 16.6.0 already out.

@kelset

This comment has been minimized.

Collaborator

kelset commented Oct 24, 2018

couple of updates:

  • Yes, as expected, 16.6.0 has been released. But the React sync commit to provide "first class" support to it in react-native is not yet out, and it may take some time (I have no further info on this)
  • because of the GitHub outage, basically CircleCI played funny so no CI jobs were run until, like, late yesterday night
  • the CI on 0.57-stable is currently red for the Android test, there is a PR that should fix it (facebook/react-native#21910) - since it's a minor change I may attempt it locally in the branch but I'm really busy today so not sure
@dvicory

This comment has been minimized.

dvicory commented Oct 24, 2018

An extra vote towards cherry-picking facebook/react-native@b002df9. We need this for react-native-navigation.

@ikesyo

This comment has been minimized.

ikesyo commented Oct 25, 2018

@matt-oakes

This comment has been minimized.

matt-oakes commented Oct 25, 2018

It would be good to get this merged PR cherry-picked as the issue is causing crashes on Android phone:

PR: facebook/react-native#20913
Commit: facebook/react-native@09c78fe

@kelset kelset referenced this issue Oct 25, 2018

Closed

0.57.4 Discussion #54

@kelset

This comment has been minimized.

Collaborator

kelset commented Oct 25, 2018

hey everyone, thanks for all the feedback and participation. CircleCI was stuck again (and I was really busy) so I preferred to move forward with the set of commits I have already tested.

0.57.4 is out (I've tested it "after release" and didn't have any issues), the new discussion is here -> #54

(I've already added the two newest commits requested there)

@kelset kelset closed this Oct 25, 2018

@react-native-community react-native-community locked as resolved and limited conversation to collaborators Oct 25, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.