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

[GENERAL] Add .nvmrc + fix node version for eslint 5 compat #20109

Closed
wants to merge 3 commits into from
Closed

[GENERAL] Add .nvmrc + fix node version for eslint 5 compat #20109

wants to merge 3 commits into from

Conversation

slorber
Copy link
Contributor

@slorber slorber commented Jul 9, 2018

A .nvmrc file is practical when working on many node projects with different version requirements.

Also, the current required node engine is incompatible with eslint5 requirements:

  "engines": {
    "node": ">=8"
  },

It produces the following warning on install:

error eslint@5.0.1: The engine "node" is incompatible with this module. Expected version "^6.14.0 || ^8.10.0 || >=9.10.0".
error Found incompatible module

I made the requirement compatible with eslint5 requirements

Test Plan:

yarn test is still passing

Release Notes:

[GENERAL] [ENHANCEMENT] Add .nvmrc and ensure node version required is compatible with ESLint 5

@slorber slorber requested a review from hramos as a code owner July 9, 2018 08:55
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 9, 2018
@pull-bot
Copy link

pull-bot commented Jul 9, 2018

Warnings
⚠️

📋 Release Notes - This PR may have incorrectly formatted Release Notes.

Generated by 🚫 dangerJS

@react-native-bot react-native-bot added ✅Test Plan Missing Changelog This PR appears to be missing a changelog, or they are incorrectly formatted. labels Jul 9, 2018
@slorber
Copy link
Contributor Author

slorber commented Jul 9, 2018

not sure exactly why ios e2e tests are failing on CI, any idea?

@janicduplessis
Copy link
Contributor

janicduplessis commented Jul 15, 2018

Tests are currently broken on master, sorry about that.

I think if we change this we'd have to bump the minimum node version from 8 to 8.10. There might be some docs to update as well as https://github.com/facebook/react-native/blob/master/local-cli/server/checkNodeVersion.js#L17.

We actually require 8.3 anyway because of some use of object destructuring.

@slorber
Copy link
Contributor Author

slorber commented Jul 16, 2018

Ok thanks, I'll update my PR with doc + checkNodeVersion to 8.10 soon

# Conflicts:
#	package.json
@slorber
Copy link
Contributor Author

slorber commented Jul 23, 2018

@janicduplessis actually not sure if I should make the changes you wanted

ReactNative can actually work for node >= 8.3, it's just ESLint which requires 8.10. So it may be a good idea to have a .nvmrc 8.10 for contributors, but not necessary to force end users to use 8.10 as they won't run ESLint anyway. I don't think the engines attribute allow that granularity between distinct version requirement for devtools and consumer code.

Maybe it's better to let engine >= 8.3 and only add nvmrc for a recommended contributor version to use?

cc @hramos who changed the version recently: https://github.com/facebook/react-native/pull/20236/files

@hramos
Copy link
Contributor

hramos commented Jul 23, 2018

@slorber that approach sounds good - let's use engine for end-user minimum required Node version, .nvmrc for contributors.

@hramos hramos added Core Team and removed Missing Changelog This PR appears to be missing a changelog, or they are incorrectly formatted. labels Jul 23, 2018
@slorber
Copy link
Contributor Author

slorber commented Jul 24, 2018

ok,

I've reverted the PR's code to only add a .nvmrc and let the engine >= 8.3

@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Jul 30, 2018
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hramos is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Collaborator

This pull request was closed by @slorber in 30b9d81.

Once this commit is added to a release, you will see the corresponding version tag below the description at 30b9d81. If the commit has a single master tag, it is not yet part of a release.

@facebook facebook locked as resolved and limited conversation to collaborators Jul 31, 2018
@react-native-bot react-native-bot added the Merged This PR has been merged. label Jul 31, 2018
@hramos hramos removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Feb 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants