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

no-extraneous-deps globs don't also check against process.cwd() + glob #602

Closed
ljharb opened this issue Oct 4, 2016 · 8 comments
Closed

Comments

@ljharb
Copy link
Member

ljharb commented Oct 4, 2016

I want to set my glob to tests/**, so that only the top-level "tests" dir can contain things that require dev deps.

However, this rule compares against the full absolute path of the file, which includes the path to the project root - which isn't really useful, and forces people to start 100% of their globs with "**/".

I'd like the rule to check both against the full filename, and also against path.join(process.cwd(), glob), to remove the need for this boilerplate in airbnb's shared config.

@jfmengels
Copy link
Collaborator

Sounds like a good idea.

The one point I'm not familiar with is whether using process.cwd() is the right way to do this. Because that would mean you would get different results based on the directory we run ESLint in, right?

I think it would be more correct to replace process.cwd() by the root of of the project (probably where package.json is located). But as we saw recently, especially with this rule, that's more complicated than expected with monorepos. So I'd probably fall back to the use of process.cwd().

Do you think that running ESLint not from the root folder is a sufficiently rare case to use process.cwd()? I always run from the root, but others may do it differently.

cc @benmosher if you have time to give your opinion on this

@ljharb
Copy link
Member Author

ljharb commented Oct 4, 2016

Yes, I don't think any command should ever be run anywhere except from the project root.

There's a thread on eslint about locating the project root, and they concluded that process.cwd was best.

@jfmengels
Copy link
Collaborator

Ok, that sounds good to me then. Help welcome!

(cc @knpwrs, if you feel like helping out again 😄)

@knpwrs
Copy link
Contributor

knpwrs commented Oct 5, 2016

@ljharb Can you link to that thread for reference?

@ljharb
Copy link
Member Author

ljharb commented Oct 5, 2016

@knpwrs
Copy link
Contributor

knpwrs commented Oct 5, 2016

So... speaking of multiple .eslintrc files how should this behave in that context? An .eslintrc file in the test directory would still execute process.cwd() in the root directory if I understand that discussion correctly.

I realize your situation is a single .eslintrc file but I'd want to support all usages.

@ljharb
Copy link
Member Author

ljharb commented Oct 6, 2016

@knpwrs yes, that's the intention. nested eslintrc files would include the path relative to the project root, not to themselves

@benmosher
Copy link
Member

Using process.cwd makes sense to me. A number of things already do this.

It can cause problems with editor integration, but I think the workarounds in those cases are well understood.

ljharb added a commit to ljharb/eslint-plugin-import that referenced this issue Oct 18, 2016
ljharb added a commit to ljharb/eslint-plugin-import that referenced this issue Oct 22, 2016
ljharb added a commit to ljharb/eslint-plugin-import that referenced this issue Oct 27, 2016
ljharb added a commit to ljharb/eslint-plugin-import that referenced this issue Oct 27, 2016
cmoesel added a commit to AHRQ-CDS/AHRQ-CDS-Connect-Authoring-Tool that referenced this issue Feb 21, 2018
When using yarn, eslint is loading an older version of eslint-plugin-import, which causes problems in the current configuration.  This commit fixes the problems by disabling the problem rule and adjusting the problem glob.

See: yarnpkg/yarn#3332

And: import-js/eslint-plugin-import#602
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

4 participants