-
Notifications
You must be signed in to change notification settings - Fork 46
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
Use env var to override local eslint location #35
Conversation
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.
I don’t think is a good solution. This entire module may not even be needed; a package like npm-which can find a local one, falling back to a global one in PATH.
Regardless of what kind of project you have, however, you absoluely need to have a package.json and node_modules to be able to participate in the modern JS ecosystem, and i don’t think supporting anything else is actually helping.
New changes - instead of using env var, just fallback to normal require('eslint'). This should be a very appropriate fallback. |
While this is better, I don't understand how this could ever work without an |
It will work if you specify ESLint in the NODE_PATH. See: https://nodejs.org/api/modules.html#modules_loading_from_the_global_folders and https://nodejs.org/api/modules.html#modules_all_together Note: jest-runner-eslint currently specifies ESLint as a dependency, so it will load from here, before going to NODE_PATH. This is fine with me, since I don't have any version requirements on ESLint, and any version is ok. But it might make sense to move ESLint from dependencies to devDependencies too. |
|
return require(path.resolve(nodeModulesPath, 'eslint')); | ||
try { | ||
// eslint-disable-next-line import/no-dynamic-require, global-require | ||
return require(path.resolve(nodeModulesPath, 'eslint')); |
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.
what about just using https://www.npmjs.com/package/resolve-from here?
Replace the whole getLocalESLint
with just a resolveFrom(config.rootDir, 'eslint');
.
I think that should be enough (and it means findUp
should not be needed)
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.
I’d prefer not to use that package due to its aggressive dropping of support of older platforms. Could resolve
work here?
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.
seems like it takes basedir
, so resolve.sync('eslint', { basedir: config.rootDir })
should in theory do the trick as well
See #42 |
The rationale behind this is we have a non-standard project set up. We have a lot of .js files that are not part of a npm project, and hence don't have any /node_modules/ anywhere in the ancestor path.
We work around this for jest transformers/resolvers by always specifying the full filesystem path to the node module, instead of just relying on the node resolution algorithm to discover modules.
When trying to use this eslint-runner, the eslint location can not be found. This is a simple change that lets one to use the env var ESLINT_PATH to override the location of the local eslint. Had do use the env, since jest doesn't pass any options to the test runners (unfortunately).