-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
docs(jdk): recommend JDK 11. JDK18 will not work. JDK17 may not work < 0.68 #3178
Conversation
I chose 14 as the max because I thought it was a well-maintained release but as I consult the version table it appears that 17 is an LTS, and currently 17 is supported. I'll modify it to recommend 11 or 17 as those are LTS versions so should receive updates through system package managers and thus be reasonable choices |
✅ Deploy Preview for react-native ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for react-native ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thx for picking this up, going w my spidey tingles in future to more proactively suggest/pr updates
@@ -28,7 +28,7 @@ brew install --cask zulu11 | |||
|
|||
The Zulu OpenJDK distribution offers JDKs for **both Intel and M1 Macs**. This will make sure your builds are faster on M1 Macs compared to using an Intel-based JDK. | |||
|
|||
If you have already installed JDK on your system, make sure it is JDK 11 or newer. | |||
If you have already installed JDK on your system, we recommend either of the two currently supported LTS versions, JDK 11 or JDK 17. JDK 18 is not supported yet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JDK 18 is not supported yet.
just chiming in agree on this, not only was it causing 0.69.0-rc.6 java.lang.OutOfMemoryError debug-mode earlier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but also separately (my template) slightly (unsupportedly 😅) ahead on Gradle/AGP 7.4.2
debug/release both build fine, debug-mode runs fine, but release has similar runtime error i'm debugging atm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dropping here for anyone interested
I think we should stringly enforce Java 11 for now. I haven't got the time to look into this specifically, but I believe JDK 17 won't work at all (for the same reason JDK 18 won't work) |
I have specific user feedback that JDK17 is working...at least mostly? - react-native-device-info/react-native-device-info#1431 (comment)
Let me check |
My react-native-firebase / react-native build qualifier has no trouble with JDK17 🤷 - https://github.com/mikehardy/rnfbdemo/blob/main/make-demo.sh - used It is always possible I made a mistake but it appears to work fine, it is an LTS, and another user confirmed it worked as well. So TL;DR: @cortinico I think JDK17 is good actually, an unexpected result on my part to be honest but happy about it since it's now the most current LTS. |
Have you tried to run it also? |
Of course! :-) I also just ran my main work project (which has a large pile of dependencies, not all current) and it seemed fine. JDK17 seems fine, with success reports from template and in work projects and no failure reports. Absence of proof (of failure) is not proof of absence (of failure) but with zero reports of failure and success in all the things I normally test I'm not sure how else to qualify it, it seems good to me |
my results btw from earlier |
Having an open upper end is always dangerous IMHO |
Feedback from previous user that prompted this PR for me is that JDK17 will work with RN68ish but rn67 is problematic for JDK17. if (big if? is it?) the docs are for new installs, then that tells me current state of PR (which recommends 11-17) is okay. If we want the docs to also be backwards compatible then it should be "use 11" I continue to assert that open-ended upper range is dangerous as current android studio tooling is not compatible with JDK18 react-native-device-info/react-native-device-info#1431 (comment) That sounds all concrete and like an unmove-able stance but really I'm just trying to work with the data I've got and be helpful. Hopefully that comes through. Cheers |
Agree. Yet having to test the whole range is also a pain.
Why was it problematic for RN67? |
Unsure exactly, it was logged in the google issue tracker which smells (warning: assumption ahead) like a gradle version compatibility thing between gradle / android gradle plugin. Perhaps it couldn't handle the new JDK17 ASTs or something I dunno
+1 on that! You know I've got a stack of doctor enhancements I'd like to work through but then firebase-ios-sdk threw the So Perhaps just say "JDK11" and stick with it, sometimes being opinionated and simple is better than complicated. I don't feel strongly but I like fewer issues in my repo. If saying "JDK11" and done works here, that works for me |
Wow, apparently using JDK18 can result in ABI breaks in distributed apps on Android 8/8.1 - invertase/react-native-firebase#6338 - so, not sure what to do about that but I definitely would not include JDK18 in any range we recommend at the moment |
- JDK17 seems to work with > 0.68+ but may have problems < 0.68 - JDK18 has known problems with all versions at the moment
That caused me to come look at this again - @cortinico I see you ❤️ed my comment about just saying "JDK11" so I altered all suggestions to just say 11 and warn about incompatibilities in higher versions with more specifics in the commit message I squashed all the commits to a single one and rebased against current main here should be good? |
Yes that's good. We can merge this. |
I get reports of build errors from users that have chosen JDK18, that JDK does not work yet and should not be included either explicitly or even as part of an acceptable range ("...or newer")
See react-native-device-info/react-native-device-info#1431 (comment)
I will add a second commit here in a moment for linux setup