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

Use env var to override local eslint location #35

Closed
wants to merge 4 commits into from

Conversation

midzelis
Copy link

@midzelis midzelis commented Mar 1, 2018

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).

Copy link
Collaborator

@ljharb ljharb left a 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.

@midzelis
Copy link
Author

midzelis commented Mar 4, 2018

New changes - instead of using env var, just fallback to normal require('eslint'). This should be a very appropriate fallback.

@ljharb
Copy link
Collaborator

ljharb commented Mar 4, 2018

While this is better, I don't understand how this could ever work without an eslint dir in a node_modules directory.

@midzelis
Copy link
Author

midzelis commented Mar 4, 2018

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.

@ljharb
Copy link
Collaborator

ljharb commented Mar 4, 2018

NODE_PATH is deprecated though, and won't be supported with ESM use cases - I also don't think we should be making changes to encourage usage of that feature, and ideally, not supporting it at all.

return require(path.resolve(nodeModulesPath, 'eslint'));
try {
// eslint-disable-next-line import/no-dynamic-require, global-require
return require(path.resolve(nodeModulesPath, 'eslint'));
Copy link
Member

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)

Copy link
Collaborator

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?

Copy link
Member

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

@SimenB
Copy link
Member

SimenB commented Apr 27, 2018

See #42

@SimenB SimenB closed this Apr 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants