Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Fails when upgrading to 3.7.0 with an error message about extends #1083

Closed
vertti opened this issue Apr 5, 2016 · 11 comments
Closed

Fails when upgrading to 3.7.0 with an error message about extends #1083

vertti opened this issue Apr 5, 2016 · 11 comments

Comments

@vertti
Copy link

vertti commented Apr 5, 2016

Bug Report

  • TSLint version: 3.7.0
  • TypeScript version: 1.8.9
  • Running TSLint via: CLI

Running

We run tslint from make file. Everything works fine with any tslint previous to 3.7.0.

tslint -c tslint.json `find src/ts -name *.ts | xargs`

I have not added anything related to the new extends feature, so my tslint.json only contains a big rules block.

Actual behavior

tslint -c tslint.json find src/ts -name *.ts | xargs
/Users/vertti/.npm-packages/lib/node_modules/tslint/lib/configuration.js:112
throw new Error(("Invalid "extends" configuration value - could not require "" + relativeFilePath + "". ") +
^

Error: Invalid "extends" configuration value - could not require "tslint.json". Review the Node lookup algorithm (https://nodejs.org/api/modules.html#modules_all_together) for the approximate method TSLint uses to find the referenced configuration file.

Expected behavior

I'm expecting normal linting.

@staltz
Copy link

staltz commented Apr 5, 2016

I can confirm this from my side too. I tried using npm@3 and npm@2, the bug happens with both. Downgrading TSLint to 3.6.0 worked for me.

@jkillian
Copy link
Contributor

jkillian commented Apr 5, 2016

Sounds like a bug 😢. I'll look into it sometime in the next few hours!

@jkillian
Copy link
Contributor

jkillian commented Apr 5, 2016

@vertti @staltz So this is actually the intended behavior, sort of. What happened is we introduced a way so that you could load configurations that were in an NPM package. This was mainly for the new and optional extends field of tslint.json files. The semantics are similar to the semantics when using require in Node:

  • An absolute path loads a file from the location it points to.
  • A relative path starting with ./ or ../ loads a file relatively.
  • A plain string value, such as "tslint-config", tries too load the main file of an npm package called "tslint-config".

What's happening here is that the -c CLI option uses the same code and logic to load config files, and thus in your cases is trying to load an npm module named "tslint.json". You can resolve this bug on your end by specifying the path as "./tslint.json".

Nonetheless, this clearly is unexpected behavior to users. I see two solutions:

  • Better education/error messages. I made a mistake when writing this error message - I was thinking it would only get thrown in the context of using extends, but clearly that is not the case. With good documentation / clear error messages, users could probably figure out to start their path with a ./.
  • Change it so that the -c option always treats its argument as a path and never attempts to resolve it as a module. Module resolution would still be available as is for the extends option.

@staltz
Copy link

staltz commented Apr 5, 2016

Shouldn't this be a breaking change version update then?

@sihu
Copy link

sihu commented Apr 5, 2016

agree with @staltz that it's not the expected, that an upgrade breaks my working version in perspective of backwards compatibility. but if you don't want to make a breaking change version update i'd like your idea of improving the error message, so i know as a user what i have to change in my configuration to make it work again.

@jkillian
Copy link
Contributor

jkillian commented Apr 5, 2016

TSLint 3.7.0 has only been published for less than a day, so I'm happy to revert this semi-unintended breaking change (via option 2 described above). I don't think this will mess up anyone's workflow so shouldn't be a problem.

I was more worried about the -c option having different semantics than the extends field, but since one is a CLI option and one is a field in a configuration file, I think slightly different semantics should be okay?

@jkillian
Copy link
Contributor

jkillian commented Apr 5, 2016

Filed #1084 to fix this

@jkillian
Copy link
Contributor

jkillian commented Apr 5, 2016

This should be fixed with version 3.7.1 which I just published. If you all are still running into any problems, let me know!

@timbru31
Copy link

I'm still having issues with 3.7.1, see https://github.com/Microsoft/vscode-tslint/issues/41

Edit: setting "tslint.configFile": "./tslint.json", instead of "tslint.configFile": "tslint.json",
in our .vscode/settings.json solved it.

@jkillian
Copy link
Contributor

Glad you found a work-around @timbru31. This is our fault and not the fault of vscode-tslint, #1093 should fix this though so that it will work correctly with or without the "./"

@jkillian
Copy link
Contributor

@timbru31 Fixed version is published (v3.7.2). Let me know if you run into any further problems

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants