Skip to content
This repository has been archived by the owner on Jan 14, 2019. It is now read-only.

feat: add errorOnTypeScriptSyntaticAndSemanticIssues option #57

Merged
merged 5 commits into from
Dec 28, 2018

Conversation

JamesHenry
Copy link
Owner

@JamesHenry JamesHenry commented Dec 21, 2018

@armano2 @j-f1 @uniqueiniquity

I have applied the "errorOnTypeScriptSyntaticAndSemanticIssues" option we discussed.

The way I have tested it is to create a new spec which goes through all the existing fixtures, and only really cares if they throw. A simple sentence is snapshotted if they don't.

I have deliberately split this PR into two commits:

  1. adds the test infrastructure, and adds the option
  2. enables the option

Therefore the diff between 1 and 2 is really important, because it is the difference between current behaviour, and behaviour with the option in enabled.

Even on initial glance, I have some concerns around the Prettier use-case, because I think the scale of "strictness" of reporting now looks like this:

Least strict: This parser with option disabled
Mid: Babel
Most strict: This parser with option enabled

For example, for this source, we will now report an error "Cannot find name 'foo'.":

foo();

Please take a look at the diff and let me know what you think! :)

@JamesHenry
Copy link
Owner Author

Oh yipes, we get some very TypeScript-specific stuff in there:

"message": "Cannot find name 'module'. Do you need to install type definitions for node? Try \`npm i @types/node\` and then add \`node\` to the types field in your tsconfig.",

@armano2
Copy link
Contributor

armano2 commented Dec 21, 2018

you have to filter messages by types you can look on this what i proposed in plugin:
https://github.com/bradzacher/eslint-plugin-typescript/pull/245/files#diff-6bbbfdcb5845fbd335b2e1137e998babR31

error.category: DiagnosticCategory.Error = 1

@armano2
Copy link
Contributor

armano2 commented Dec 21, 2018

additionally messageText can be an object when errors are nested and its going to lead to duplicates

// example of code with nested errors :)
interface Foo {
  hello: string;
}
const foo: string = ({ hello: 2 } as Foo)!.foo

@j-f1
Copy link
Contributor

j-f1 commented Dec 21, 2018

How does it change if you only error on syntactic issues?

@JamesHenry
Copy link
Owner Author

@j-f1 Hopefully @uniqueiniquity can tell us for sure, but I think getSyntacticDiagnostics is always returning nothing because those are covered by the long-standing logic of looking at the parseDiagnostics array on the AST

@JamesHenry
Copy link
Owner Author

Thanks @armano2, I have now seen the ts.flattenDiagnosticMessageText helper

I had a quick look and they all seem to come through with category 1:

enum DiagnosticCategory {
        Warning = 0,
        Error = 1,
        Suggestion = 2,
        Message = 3
    }

Unfortunately, it looks like code is just a number, so we will have to infer what they mean and filter the list ourselves 🤔

@JamesHenry
Copy link
Owner Author

I can see TypeScript bundles a file:

node_modules/typescript/lib/diagnosticMessages.generated.json

Which looks like this:

{
  "Unterminated_string_literal_1002" : "Unterminated string literal.",
  "Identifier_expected_1003" : "Identifier expected.",
  "_0_expected_1005" : "'{0}' expected.",
  "A_file_cannot_have_a_reference_to_itself_1006" : "A file cannot have a reference to itself.",
  "Trailing_comma_not_allowed_1009" : "Trailing comma not allowed.",
  "Asterisk_Slash_expected_1010" : "'*/' expected.",
  "An_element_access_expression_should_take_an_argument_1011" : "An element access expression should take an argument.",
  "Unexpected_token_1012" : "Unexpected token.",
  "A_rest_parameter_or_binding_pattern_may_not_have_a_trailing_comma_1013" : "A rest parameter or binding pattern may not have a trailing comma.",
...

@armano2
Copy link
Contributor

armano2 commented Dec 21, 2018

@JamesHenry i was doing some tests with code from my company and i got some suggestions and few warnings, i think it's better to filter them 😄

@uniqueiniquity
Copy link
Contributor

I don't think there's an easy way to filter out the "very TS specific errors" (e.g., couldn't find @types for imported package). One could filter out by error code, since those (I believe) reliably won't change, but there's a lot of errors. Your intuition about why you're not getting anything from getSyntacticDiagnostics is correct, since that method generally just returns the parseDiagnostics array. I will note, though, that it also performs some additional work if the file is a JS file, so it's probably still worth using.

@JamesHenry
Copy link
Owner Author

I think I will go for an approach of filtering out all of the errors initially, and then we can opt into errors we know we want to provide feedback on (when the flag is enabled), we can then also extend this easily over time, and then actual errors we will care about should hopefully be much more manageable

@JamesHenry JamesHenry force-pushed the errorOnTypeScriptSyntaticAndSemanticIssues branch from 0ccfc84 to 1e0a824 Compare December 27, 2018 14:43
@JamesHenry
Copy link
Owner Author

JamesHenry commented Dec 27, 2018

Updated as per that idea, particularly important now is commits three, as it shows what happens before and after a particular error is supported.

I went with the case previously outlined here: eslint/typescript-eslint-parser#547

As a starting point

cc @mysticatea

@JamesHenry JamesHenry force-pushed the errorOnTypeScriptSyntaticAndSemanticIssues branch from 1e0a824 to 98acfe1 Compare December 28, 2018 17:49
@JamesHenry JamesHenry merged commit 36d2681 into master Dec 28, 2018
@JamesHenry JamesHenry deleted the errorOnTypeScriptSyntaticAndSemanticIssues branch December 28, 2018 18:19
@JamesHenry
Copy link
Owner Author

🎉 This PR is included in version 8.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@armano2
Copy link
Contributor

armano2 commented Dec 29, 2018

we should add this to readme / how to use it

@armano2
Copy link
Contributor

armano2 commented Jan 4, 2019

i just notice that there is typo in this PR it should be syntactic not syntatic xd

@JamesHenry
Copy link
Owner Author

Whoops 😂 at least it is undocumented, so we can sneak a fix in before it is

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

Successfully merging this pull request may close these issues.

4 participants