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

regression in 2.13.0 release - relative extend fails to find referenced file #6489

Closed
travi opened this issue Jun 21, 2016 · 18 comments
Closed
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion

Comments

@travi
Copy link

travi commented Jun 21, 2016

After the release of 2.13.0, projects consuming my personal eslint-config can no longer successfully load the chain of extends. It would appear that this issue is related to #6450, but the fix for that specific issue (released in 2.13.1) does not appear to resolve this issue.

What version of ESLint are you using?
2.13.1

What parser (default, Babel-ESLint, etc.) are you using?
grunt-eslint

Please show your full configuration:

What did you do? Please include the actual source code causing the issue.
No eslint related changes were made directly to my projects. I do not track a direct dependency on eslint in the projects that use grunt-eslint, and grunt-eslint depends on ^2.0.0. Since I do not have travis-ci configured to cache dependencies between builds, when it built contributions after 2.13.0 was released it was immediately pulled into the next build and caused it to fail.

What did you expect to happen?
I expected that the ^2.0.0 range would be safe from breaking changes and not require my projects to depend on a specific eslint version since I do not depend directly on eslint, but rather depend on it only through grunt-eslint.

What actually happened? Please include the actual, raw output from ESLint.
https://travis-ci.org/travi/admin.travi.org-components/jobs/138631754

Running "eslint:target" (eslint) task
Warning: Cannot read config file: /home/travis/build/travi/admin.travi.org-components/node_modules/@travi/eslint-config-travi/rules/react
Error: ENOENT: no such file or directory, open '/home/travis/build/travi/admin.travi.org-components/node_modules/@travi/eslint-config-travi/rules/react'
Referenced from: @travi/travi/rules/tests/react
Referenced from: /home/travis/build/travi/admin.travi.org-components/test/unit/.eslintrc.yml� Use --force to continue.
Aborted due to warnings.
@eslintbot eslintbot added the triage An ESLint team member will look at this issue soon label Jun 21, 2016
@ilyavolodin
Copy link
Member

Can you provide full directory where https://github.com/travi/eslint-config-travi/blob/master/rules/tests/react.js#L2 is located on your build system? Right now, based, on the information you posted, it looks like ESLint is doing the right thing and your configuration is incorrect for some reason.
I assume that file is going to be located at /home/travis/build/travi/admin.travi.org-components/node_modules/@travi/eslint-config-travi/rules/tests/react.js it says that it extends react file from the parent directory which is /home/travis/build/travi/admin.travi.org-components/node_modules/@travi/eslint-config-travi/rules/react which cannot be found.

@ilyavolodin ilyavolodin added bug ESLint is working incorrectly core Relates to ESLint's core APIs and features evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Jun 21, 2016
@travi
Copy link
Author

travi commented Jun 21, 2016

correct, the file would be located at /home/travis/build/travi/admin.travi.org-components/node_modules/@travi/eslint-config-travi/rules/react.js. as with other file references in node, the .js extension is not necessary (or at least was not before the 2.13.0 release).

maybe i'm missing something from your question, but i don't understand what you are saying would be wrong with my config, especially since it has not changed.

@ilyavolodin
Copy link
Member

@travi Just to confirm, you do have a file located at /home/travis/build/travi/admin.travi.org-components/node_modules/@travi/eslint-config-travi/rules/react.js? It's possible that we introduced a regression that doesn't append extensions correctly (although I'm not sure how, that code wasn't changed).
This wasn't a feature update, we found a bug in a way that extends works and fixed it. It might've introduced a new bug, or it might've fixed something in such a way, that affected your configuration.

@travi
Copy link
Author

travi commented Jun 21, 2016

i have no way to confirm on travis without ssh access, but i can confirm locally where i see the same issue and yes, that file exists the same way it exists in the file structure on github as linked in the original post above. this fails in the same way in both 2.13.0 and 2.13.1

in addition, my team uses this same type of configuration for other private projects, but with a different eslint-config-{team-name}. we saw this same issue on our private builds today until we specified a version before 2.13.0

@travi
Copy link
Author

travi commented Jun 21, 2016

since all of the listed projects are public, please feel free to clone https://github.com/travi/admin.travi.org-components and run npm test. that should reproduce the error.

@mysticatea
Copy link
Member

mysticatea commented Jun 21, 2016

Actually, we cannot omit .js extension if we are using relative paths. We are using require("path").resolve function if it's a file path.
If the value of extends is not a file path, we are using module resolver.

But until 2.11.1, as a bug, there seemed to be cases a file path is handled as a module.

On 2.11.1

  eslint:config-file Loading JSON config file: C:\Users\t-nagashima.AD\Documents\GitHub\sandbox\.eslintrc.json +1ms
  eslint:config-file Loading @travi/travi/rules/tests/react +2ms
  eslint:config-file Attempting to resolve @travi/eslint-config-travi/rules/tests/react +1ms
  eslint:config-file Loading JS config file: C:\Users\t-nagashima.AD\Documents\GitHub\sandbox\node_modules\@travi\eslint-config-travi\rules\tests\react.js +2ms
  eslint:config-file Loading @travi\travi\rules\react +2ms
  eslint:config-file Attempting to resolve @travi/eslint-config-travi/rules/react +1ms

On 2.13.0

  eslint:config-file Loading JSON config file: C:\Users\t-nagashima.AD\Documents\GitHub\sandbox\.eslintrc.json +1ms
  eslint:config-file Loading @travi/travi/rules/tests/react +2ms
  eslint:config-file Attempting to resolve @travi/eslint-config-travi/rules/tests/react +0ms
  eslint:config-file Loading JS config file: C:\Users\t-nagashima.AD\Documents\GitHub\sandbox\node_modules\@travi\eslint-config-travi\rules\tests\react.js +2ms
  eslint:config-file Loading C:\Users\t-nagashima.AD\Documents\GitHub\sandbox\node_modules\@travi\eslint-config-travi\rules\react +2ms
  eslint:config-file Loading config file: C:\Users\t-nagashima.AD\Documents\GitHub\sandbox\node_modules\@travi\eslint-config-travi\rules\react +1ms
  eslint:config-file Error reading YAML file: C:\Users\t-nagashima.AD\Documents\GitHub\sandbox\node_modules\@travi\eslint-config-travi\rules\react +135ms
Cannot read config file: C:\Users\t-nagashima.AD\Documents\GitHub\sandbox\node_modules\@travi\eslint-config-travi\rules\react

@travi
Copy link
Author

travi commented Jun 21, 2016

fair enough. that at least makes sense why it worked before. i just published a new patch version with this change and things do appear to be working again.

however, the bit that i guess i still do not understand is why the extension is needed within the config, but not in the yaml file in the consuming project, like https://github.com/travi/admin.travi.org-components/blob/master/test/unit/.eslintrc.yml. should i need to include the file extension there as well? (again, it is working without the extension there)

@mysticatea
Copy link
Member

@travi/travi/rules/tests/mocha is a kind of module paths.
It finds the module @travi/eslint-config-travi at first, then picks up specified file from the module.
So it's the work of module resolver. Module resolver behaves similar to require().

I think that making extensions omittable is reasonable.
@eslint/eslint-team what do you think?

@ilyavolodin
Copy link
Member

If we can make it work without extensions (and without having to check each supported extension manually), I think that would be helpful.

@nzakas nzakas added enhancement This change enhances an existing feature of ESLint and removed bug ESLint is working incorrectly labels Jun 21, 2016
@nzakas
Copy link
Member

nzakas commented Jun 21, 2016

Extensions can't really be optional. You could only load .js files without the extension (theoretically). But I don't know of a way of doing that without going through full module resolution (so ../foo could be ../foo.js, ../foo/index.js, or something else if ../foo/package.json exists). Is that what we are talking about here?

@travi
Copy link
Author

travi commented Jun 21, 2016

I'm fine with adding the extension into the config. It was just a confusion point that I stumbled on because I modeled the extends reference in the config after the one I was used to in the .eslintrc.yml files in consumers that did not use extensions. Since it worked pre-2.13, I had no idea that it was not "correct".

@nzakas
Copy link
Member

nzakas commented Jun 21, 2016

Yeah, sorry about the confusion. This tends to happen when there's a bug fixed and people were relying on the bug for something. That doesn't mean we can't decide to bless that behavior, my point is just that it's a more complex problem than just, "make extensions optional."

@mattfysh
Copy link

@nzakas I'm not sure this is entirely about making extensions optional, I was under the impression processing paths via require.resolve was considered best practice when working with node packages?

@nzakas
Copy link
Member

nzakas commented Jun 22, 2016

@mattfysh we can't use require.resolve because we need to map from config file locations. The config file could be yaml or json, and require.resolve always resolves from where it is called.

@jamesplease
Copy link

jamesplease commented Jul 19, 2016

@travi (or anyone else who ran into this), is this still an issue for you in 3.x?

@travi
Copy link
Author

travi commented Jul 19, 2016

based on where this conversation ended, it did not sound likely that the behavior would be able to change so i simply updated my config to include the extension. has something changed to make it more flexible?

@nzakas
Copy link
Member

nzakas commented Jul 20, 2016

Sorry, lost track of this. Yeah, I think we're at the point where we're not going to make a change here. I think asking people to include extensions is reasonable given how we have to support a few different config file formats. Leaving open a couple more days to see if anyone else on the team feels strongly otherwise.

@nzakas
Copy link
Member

nzakas commented Aug 3, 2016

No further comments, so closing.

@nzakas nzakas closed this as completed Aug 3, 2016
travi added a commit to Dwolla/eslint-config-dwolla that referenced this issue Aug 9, 2016
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion
Projects
None yet
Development

No branches or pull requests

7 participants