Skip to content
This repository has been archived by the owner on Feb 18, 2024. It is now read-only.

Update the suggested .eslintrc.js shim so it can find .neutrinorc.js from any location #703

Merged
merged 5 commits into from
Apr 24, 2018

Conversation

mdboom
Copy link
Contributor

@mdboom mdboom commented Feb 2, 2018

When emacs flycheck package runs eslint, it runs it from the same directory as the source file. If that's the case, the suggested .eslintrc.js shim in this file will fail to find the .neutrinorc.js file. This simple fix just looks for it in the same directory as the .eslintrc.js file. This shouldn't (hopefully) have any negative consequences for any IDEs that run eslint from the project root that are currently working.

When emacs flycheck package runs `eslint`, it runs it from the same directory as the source file.  If that's the case, the suggested `.eslintrc.js` shim in this file will fail to find the `.neutrinorc.js` file.  This simple fix just looks for it in the same directory as the `.eslintrc.js` file.  This shouldn't (hopefully) have any negative consequences for any IDEs that run `eslint` from the project root that are currently working.
@constgen
Copy link
Contributor

constgen commented Feb 5, 2018

Have the same issue in VSCode. But I tried this fix in VSCode and it produces another issue

[Info  - 15:04:37] ESLint library loaded from: d:\Work\Projects_AtomSpace\Misha\frontend-js-snake\node_modules\eslint\lib\api.js
[Error - 15:04:43] Cannot read config file: d:\Work\Projects_AtomSpace\Misha\frontend-js-snake\.eslintrc.js Error: Cannot find module 'C:\Program Files (x86)\Microsoft VS Code\package.json'

I can confirm that code editors ESLint highlighting doesn't work in the current version

@@ -259,7 +259,7 @@ const { Neutrino } = require('neutrino');
// Even if using .neutrinorc.js, you must specify it when using
// the API
module.exports = Neutrino()
.use('.neutrinorc.js')
.use(__dirname + '/.neutrinorc.js')
Copy link
Member

Choose a reason for hiding this comment

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

I think the more correct solution here would be to pass the correct working directory to Neutrino so it can resolve anything else from the root dir:

module.exports = Neutrino({ root: __dirname })
  .use('.neutrinorc.js')
  .call('eslintrc');

Copy link
Contributor Author

@mdboom mdboom Feb 6, 2018

Choose a reason for hiding this comment

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

Cool, I didn't know about that. I'll update this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried this. In VSCode I get again

[Error - 12:07:54] Cannot read config file: d:\Work\Projects_Fabrica\frontend-js-snake\.eslintrc.js Error: Cannot find module 'C:\Program Files (x86)\Microsoft VS Code\package.json'

@constgen
Copy link
Contributor

constgen commented Feb 10, 2018

@mdboom did it really work for you. It didn't for me. Can you test it in VSCode (with ESLint extension installed) on your side?

@mdboom
Copy link
Contributor Author

mdboom commented Feb 12, 2018

@constgen: It works for me in emacs. I'm not a VSCode user.

@mdboom
Copy link
Contributor Author

mdboom commented Feb 12, 2018

For clarity: @constgen: Does it work for you before this change and not after, or in neither case?

@constgen
Copy link
Contributor

@mdboom It didn't work in both: before and after. I'm also looking for a solution.

@edmorley edmorley added the docs label Mar 10, 2018
@constgen
Copy link
Contributor

Have spent some time on research and debugging and found the origin of the issue on my side. It was a project setting of ESLint VS Code Extension that look like this.

"eslint.workingDirectories": [
    "./src",
    "./test"
]

I don't remember already why it was necessary. But removing this fixes the issue. This setting changes cwd of ESLint process that is started by an editor. And process.cwd() call in presets was incorrect. Of course Neutrino({ root: __dirname }) is required and stays as it fixes another issue.

So I confirm that it works on Windows now. Please review.

Copy link
Member

@edmorley edmorley left a comment

Choose a reason for hiding this comment

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

Hi! Thank you for the PR.

Could you make the same changes to the other files that reference the suggested .eslintrc.js usage?
https://github.com/mozilla-neutrino/neutrino-dev/blob/f40ea20c2eead5f13c7ebf0f22af5219a0d00b31/packages/airbnb-base/README.md#eslintrc-config
https://github.com/mozilla-neutrino/neutrino-dev/blob/f40ea20c2eead5f13c7ebf0f22af5219a0d00b31/packages/airbnb/README.md#eslintrc-config
https://github.com/mozilla-neutrino/neutrino-dev/blob/f40ea20c2eead5f13c7ebf0f22af5219a0d00b31/packages/eslint/README.md#eslintrc-config
https://github.com/mozilla-neutrino/neutrino-dev/blob/f40ea20c2eead5f13c7ebf0f22af5219a0d00b31/packages/standardjs/README.md#eslintrc-config
https://github.com/mozilla-neutrino/neutrino-dev/blob/f40ea20c2eead5f13c7ebf0f22af5219a0d00b31/docs/packages/airbnb-base/README.md#eslintrc-config
https://github.com/mozilla-neutrino/neutrino-dev/blob/f40ea20c2eead5f13c7ebf0f22af5219a0d00b31/docs/packages/airbnb/README.md#eslintrc-config
https://github.com/mozilla-neutrino/neutrino-dev/blob/f40ea20c2eead5f13c7ebf0f22af5219a0d00b31/docs/packages/eslint/README.md#eslintrc-config
https://github.com/mozilla-neutrino/neutrino-dev/blob/f40ea20c2eead5f13c7ebf0f22af5219a0d00b31/docs/packages/standardjs/README.md#eslintrc-config
https://github.com/mozilla-neutrino/neutrino-dev/blob/cc412d0c18dbaa66bceb011ed2f4185b2dbad8d2/docs/api/README.md#callcommandname
https://github.com/mozilla-neutrino/neutrino-dev/blob/dc0eb1c705ad18cc187d2da737973d75f581ce7c/docs/migration-guide.md#neutrino-v7-to-v8

@mdboom
Copy link
Contributor Author

mdboom commented Apr 2, 2018

I've updated this to add additional instances of this change as listed above.

Copy link
Member

@edmorley edmorley left a comment

Choose a reason for hiding this comment

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

Thank you for the changes.

There are a few instances remaining - I don't suppose you could fix them up too? The docs/ and packages/ versions of the docs also need to stay in sync.

neutrino-dev $ ag "module\.exports = Neutrino\(\)[^;]+\.call\('eslintrc'\)"

docs/packages/airbnb/README.md
275:module.exports = Neutrino()
276:  .use('@neutrinojs/airbnb', {
277:    eslint: {
278:      rules: { semi: 'off' }
279:    }
280:  })
281:  .use('@neutrinojs/react')
282:  .call('eslintrc');

docs/packages/airbnb-base/README.md
264:module.exports = Neutrino()
265:  .use('.neutrinorc.js')
266:  .call('eslintrc');
278:module.exports = Neutrino()
279:  .use('@neutrinojs/airbnb-base', {
280:    eslint: {
281:      rules: { semi: 'off' }
282:    }
283:  })
284:  .use('@neutrinojs/react')
285:  .call('eslintrc');

docs/packages/eslint/README.md
148:module.exports = Neutrino()
149:  .use('.neutrinorc.js')
150:  .call('eslintrc');
159:module.exports = Neutrino()
160:  .use('@neutrinojs/eslint', {
161:    eslint: {
162:      rules: { semi: 'off' }
163:    }
164:  })
165:  .call('eslintrc');

docs/packages/standardjs/README.md
262:module.exports = Neutrino()
263:  .use('.neutrinorc.js')
264:  .call('eslintrc');
273:module.exports = Neutrino()
274:  .use('@neutrinojs/standardjs', {
275:    eslint: {
276:      rules: { semi: 'error' }
277:    }
278:  })
279:  .use('@neutrinojs/react')
280:  .call('eslintrc');

@eliperelman
Copy link
Member

I know it's nitpicky, but we could also drop the .js from the .use call since Node.js will pick it up by default. Thoughts?

@edmorley
Copy link
Member

edmorley commented Apr 4, 2018

I think I'd rather leave it, since it makes it clearer to the user what file is being referenced (people are perhaps used to knowing about the implicit file extension support for imports, but it's not entirely clear that the argument being passed takes advantage of node module resolution).

@eliperelman
Copy link
Member

OK, I think that's a fair point; we can leave it.

@mdboom
Copy link
Contributor Author

mdboom commented Apr 20, 2018

I've added what are hopefully the last instances remaining.

Copy link
Member

@edmorley edmorley left a comment

Choose a reason for hiding this comment

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

Many thanks!

@edmorley edmorley merged commit 89cbce4 into neutrinojs:master Apr 24, 2018
edmorley added a commit that referenced this pull request Apr 27, 2018
Applies the #703 docs recommendation to our own .`eslintrc.js`'s too.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

4 participants