JamesHenry / typescript-estree

:sparkles: A parser that converts TypeScript source code into an ESTree-compatible form
https://jameshenry.blog
Other
84 stars 13 forks source link

Parser service is not using 'tsconfig.json' #50

Closed armano2 closed 5 years ago

armano2 commented 5 years ago

What version of TypeScript are you using? ~3.2.1

What version of typescript-estree are you using? 5.3.0

What code were you trying to parse?

var foo = parseInt("5.5", 10) + 10;
{
    "compilerOptions": {
        "target": "es5",
        "module": "commonjs",
        "strict": true,
        "esModuleInterop": true,
        "lib": [
            "es2015",
            "es2017"
        ]
    }
}

What did you expect to happen? typeChecker has no informations about types from es2015.core

Demo https://github.com/bradzacher/eslint-plugin-typescript/pull/230

Related to https://github.com/JamesHenry/typescript-estree/pull/22 https://github.com/eslint/typescript-eslint-parser/pull/568

armano2 commented 5 years ago

Update:

issue happens only when filePath is not specified or when its not pointing to real file (eslint ruleTester)

uniqueiniquity commented 5 years ago

@armano2 I'll take a look

armano2 commented 5 years ago

solution:

uniqueiniquity commented 5 years ago

What do you mean by "passing parser configuration"? The way the TypeScript compiler test harness handles this is by providing a virtual file system and copy of the library. Is there a more limited approach that you have in mind?

armano2 commented 5 years ago
// create compiler host
    const watchCompilerHost = ts.createWatchCompilerHost(
      tsconfigPath,
      /*optionsToExtend*/ undefined,
      ts.sys,
      ts.createSemanticDiagnosticsBuilderProgram,
      diagnosticReporter,
      /*reportWatchStatus*/ () => {}
    );

/*optionsToExtend*/ undefined,

armano2 commented 5 years ago

I have issues with making test for eslint in RuleTester, because i'm unable to provide configuration file (i'm not making files there, it's just code)

https://github.com/bradzacher/eslint-plugin-typescript/blob/39130b28f3d6e7bb360e50b2813a21254e86f43c/tests/lib/rules/restrict-plus-operands.js

right now to have access to libraries i had to specify empty file and tsconfig in directory.

uniqueiniquity commented 5 years ago

It appears to me that even with the ability to pass TypeScript compiler options there, the problem still occurs. In particular, when passing an object with the lib option set, it doesn't look for the correct paths. Were you able to get it to work this way?

Instead of giving typescript-estree the ability to take a config object, I'd prefer that we try to set up the eslint-plugin-typescript test infrastructure to better support this. We could either do that with your approach, with an empty.ts file and a tsconfig.json that includes it, or with a more complicated virtual file system.

armano2 commented 5 years ago

i think issue is somewhere else, we should execute createCompilerHost instead of createWatchCompilerHost when instead of file we are getting sourceCode:

function createCompilerHost(options: CompilerOptions, setParentNodes?: boolean): CompilerHost;
uniqueiniquity commented 5 years ago

How would that help? We do actually need a WatchCompilerHost in order to efficiently handle multiple parse operations over a single process lifetime.

armano2 commented 5 years ago

WatchCompilerHost is working correctly when there is no file provided. And this is correct when we want to watch for file changes.

uniqueiniquity commented 5 years ago

I'm not sure I understand you completely. Let me try to summarize what I think you are saying; please tell me if I am not correct.

MOTIVATION: In eslint-plugin-typescript, we want to test semantic rules. However, test cases that use functions from lib don't work as easily as existing test cases. This is because typescript-estree requires an actual tsconfig.json file to specify compiler options, and for those options to apply, the test file must exist and be included by the tsconfig.json.

PROPOSAL: Your suggestion is to change the typescript-estree API so that, as an alternative to reading a particular tsconfig.json file, users of this package can provide a TypeScript configuration options object to the parse method.

CONCERNS: I think this will only solve the issue of needing a real tsconfig.json object for the test runner in eslint-plugin-typescript. As far as I have seen, the lib files do not resolve properly when they are provided through optionsToExtend in createWatchCompilerhost.

Regarding your most recent comment, shouldProvideParserServices is not a recognized option for typescript-estree. It is only an internal flag for distinguishing between a call to generateAST through parse versus parseAndGenerateServices. The tests in https://github.com/JamesHenry/typescript-estree/blob/master/tests/lib/semanticInfo.ts do use parseAndGenerateServices, and therefore do have shouldGenerateServices and project set (and as a result, have shouldProvideParserServices set as well).

I am certainly open to reviewing changes to typescript-estree to address this issue, though I think @JamesHenry should weigh in on any API changes. However, since this issue seems specific to the eslint-plugin-typescript test infrastructure, I think we should try to resolve it in that package.

armano2 commented 5 years ago

@uniqueiniquity i did some testings with setup with vue-eslint-parser + typescript-eslint-parser and looks like this issue is present

vue-eslint-parser is extracting script part of code and passing it to parser specified in configuration. parserOptions are correctly propagated to typescript-eslint-parser but because files are "virtual" configuration is not working.


its going to be same case for https://github.com/BenoitZugmeyer/eslint-plugin-html (i still have to test it)

uniqueiniquity commented 5 years ago

I see. I'm starting to see the underlying issue here. I'm sorry it took so long for me to understand :)

I'm looking into ways to resolve this now. I'm worried there might not be a way to handle every case by changing typescript-estree, but I'm working on it now.

JamesHenry commented 5 years ago

@uniqueiniquity Did #53 do enough to warrant closing this now? Or are there further things to work on?

uniqueiniquity commented 5 years ago

Yep, my understanding is that #53 addressed the issues discussed here.