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

Explicitly specify ESLint config path for editor plugins in package.json #149

Merged
merged 2 commits into from
Jul 24, 2016

Conversation

keyz
Copy link
Contributor

@keyz keyz commented Jul 24, 2016

Fixes #124. Notice that we need to change the path in package.json while ejecting since the config folder gets moved.

Tested with Atom (Nuclide) and Visual Studio Code and it works both before and after ejection. I'm not sure about Sublime Text -- seems like SublimeLinter-eslint only recognizes .eslintrc config. However, it could be fixed by configuring the linter plugin itself. Or even better, could someone who uses Sublime Text send a PR to let it recognize the eslintConfig field in package.json? :)

Edit: @kevinastone confirmed that Sublime Text works fine too.

@mxstbr
Copy link
Contributor

mxstbr commented Jul 24, 2016

Awesome! 👍

@@ -104,6 +104,11 @@ prompt('Are you sure you want to eject? This action is permanent. [y/N]', functi
});
delete hostPackage.scripts['eject'];

// explicitly specify ESLint config path for editor plugins
hostPackage.eslintConfig = {
extends: "./config/eslint.js",
Copy link
Contributor

Choose a reason for hiding this comment

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

I might be totally wrong, but is it safe to use / since it's possible to be \ on windows? I think path.resolve works cross platform.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @eanplatter, I didn't notice that!

However, path.resolve produces an absolute path which is probably not what we want here (what if the user moves or renames the project folder?).

How about changing it to path.normalize('./') + path.normalize('config/eslint.js')? The first ./ (.\\ on windows) is required since Atom doesn't work without it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@insin could you help to confirm if this is an issue on windows? Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

I actually think it won't be, Node fs functions treat / in platform independent way. I wrote some unnecessary path.joins in the code before I learned this.

Copy link
Contributor

@insin insin Jul 24, 2016

Choose a reason for hiding this comment

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

Yeah, / is fine in Node.js as an input path like this.

You only get problems if you're doing path stuff manually with / and you might have a path which has been made "native" by converting it to absolute or relative with path (which is why checking an absolute path with a RegExp can be a gotcha - 🎵 Gotta [\\/] 'Em All, Pathémon 🎵)

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome, thanks for clearing that up for me!

@ghost ghost added the CLA Signed label Jul 24, 2016
@kevinastone
Copy link

Sublime Text works as well with the eslintConfig within the package.json.

@gaearon
Copy link
Contributor

gaearon commented Jul 24, 2016

Let’s remove unnecessary ./node_modules/ and I think this is good to go.

@keyz
Copy link
Contributor Author

keyz commented Jul 24, 2016

@gaearon haha i was just about to reply to you -- it doesn't work without ./node_modules/, at least for Nuclide. I think the issue here is editor plugins resolve the config file themselves and they might not actually use node's resolve.

@gaearon
Copy link
Contributor

gaearon commented Jul 24, 2016

Oh okay then.

@gaearon gaearon merged commit 76935de into facebook:master Jul 24, 2016
@gaearon
Copy link
Contributor

gaearon commented Jul 24, 2016

Thanks.

@keyz keyz deleted the eslint-in-package-json branch July 24, 2016 21:33
@mareksuscak
Copy link
Contributor

FYI: I know this has already been merged but it only works when you install eslint and all the plugins globally. AFAIK it's a good rule of thumb to install all the dependencies locally per project.

@gaearon gaearon added this to the 0.2.0 milestone Jul 25, 2016
gaearon added a commit that referenced this pull request Jul 25, 2016
This way the one generated by `init.js` doesn’t get overwritten later.
This fixes #149 which got broken as part of #94
@gaearon gaearon mentioned this pull request Jul 27, 2016
@weisjohn
Copy link
Contributor

I'm trying to use this, and even when installing globally as @mareksuscak mentioned, I get an error in Atom:

ESLint
Error
Fatal Parsing error: The keyword 'import' is reservedat line 1 col 1
File 1Project 11 Issuesrc/App.js22:1
LFUTF-8JavaScriptfeature/boilerplate2+22
Error: Failed to load plugin react: Cannot find module 'eslint-plugin-react' Referenced from: /Users/john/mysrc/create-test/package.json
Error: Failed to load plugin react: Cannot find module 'eslint-plugin-react'
Referenced from: /Users/john/mysrc/create-test/package.json
    at Function.Module._resolveFilename (module.js:338:15)
    at Function.Module._load (module.js:289:25)
    at Module.require (module.js:366:17)
    at require (module.js:385:17)
    at Object.module.exports.load (/Users/john/.atom/packages/linter-eslint/node_modules/eslint/lib/config/plugins.js:114:26)
    at Array.forEach (native)
    at Object.module.exports.loadAll (/Users/john/.atom/packages/linter-eslint/node_modules/eslint/lib/config/plugins.js:136:21)
    at load (/Users/john/.atom/packages/linter-eslint/node_modules/eslint/lib/config/config-file.js:505:21)
    at /Users/john/.atom/packages/linter-eslint/node_modules/eslint/lib/config/config-file.js:392:36
    at Array.reduceRight (native)

@weisjohn
Copy link
Contributor

If I symlink eslint.js -> node_modules/react-scripts/config/eslint.js and install eslint, eslint-plugin-react, and babel-eslint, things and then modify my package.json accordingly:

  "eslintConfig": {
    "extends": "./eslint.js"
  }

then all is well.

@keyz
Copy link
Contributor Author

keyz commented Jul 27, 2016

@mareksuscak sorry just saw this. I don't have a global ESLint or ESLint plugins installed and my Nuclide/Visual Studio Code works fine. Could you tell me more about your experience (editor, plugin, setup, etc.)? Thanks.

Edit: sorry you're right! Please track #247.

@gaearon
Copy link
Contributor

gaearon commented Jul 27, 2016

@weisjohn Are you using npm 3? The fix would only work with it.

@weisjohn
Copy link
Contributor

@gaearon

$ node -v && npm -v
v5.12.0
3.8.6

kalekseev pushed a commit to kalekseev/create-react-app that referenced this pull request Sep 11, 2017
@lock lock bot locked and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Not compatible with VS Code + ESLint plugin
9 participants