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

feat(typescript-estree)!: throw error on file not in project when project set #760

Merged

Conversation

uniqueiniquity
Copy link
Contributor

@uniqueiniquity uniqueiniquity commented Jul 25, 2019

BREAKING CHANGE

Currently, when using @typescript-eslint/parser with the project option, there is no connection between the set of files as defined by the tsconfig.json file(s) you provide and the files that you're actually linting. It turns out that if you lint a file not in that set, the process of building enough context to drive semantic rules incurs a significant performance hit.

Here, we introduce an error when such a file is linted when the project setting is provided. This will help users avoid incurring this performance hit, while explicitly alerting them of what issue is occurring (rather than simply not running semantic lint rules on such files). Note that this error matches TSLint's behavior in this scenario.

Fixes #724

@uniqueiniquity uniqueiniquity changed the title docs(parser): update README with new error feat(typescript-estree): throw error on file not in project when project set Jul 25, 2019
@codecov
Copy link

codecov bot commented Jul 25, 2019

Codecov Report

Merging #760 into master will decrease coverage by 0.05%.
The diff coverage is 92.3%.

@@            Coverage Diff            @@
##           master    #760      +/-   ##
=========================================
- Coverage   94.26%   94.2%   -0.06%     
=========================================
  Files         112     112              
  Lines        4793    4801       +8     
  Branches     1333    1334       +1     
=========================================
+ Hits         4518    4523       +5     
- Misses        158     159       +1     
- Partials      117     119       +2
Impacted Files Coverage Δ
packages/typescript-estree/src/tsconfig-parser.ts 87.83% <100%> (-0.74%) ⬇️
packages/typescript-estree/src/parser.ts 93.54% <88.88%> (-1.46%) ⬇️

@bradzacher bradzacher added the breaking change This change will require a new major version to be released label Jul 25, 2019
Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for doing this!

we probably need to test this with a vue project, just so we can see how this interacts with vue-eslint-parser.

code itself looks good for me.

packages/parser/README.md Outdated Show resolved Hide resolved
packages/typescript-estree/src/parser.ts Outdated Show resolved Hide resolved
@bradzacher bradzacher changed the title feat(typescript-estree): throw error on file not in project when project set feat(typescript-estree)!: throw error on file not in project when project set Jul 25, 2019
@uniqueiniquity
Copy link
Contributor Author

@bradzacher I've addressed your comments and synced with master.
The tests for no-unnecessary-assertion are totally busted on my machine but passing fine here... with or without my most recent commits. I'll probably delete the whole folder and try again locally.

I haven't had a chance to try with vue yet, but I will as soon as I can (tomorrow or this weekend).
I'm not concerned, though, as long as the vue parser is giving us typescript code to parse with the .vue filename, and as long as projects are set up as the vetur documentation suggests.

@uniqueiniquity
Copy link
Contributor Author

uniqueiniquity commented Jul 26, 2019

Actually, just tried it with a project generated by the Vue CLI and hit an issue.
Will investigate more when less tired :)

Edit: Couldn't help myself from looking more.
Seems fine to me.
Here's what I did:

  • Using the Vue CLI, vue create hello-world with babel, eslint, and typescript
  • set the parserOptions in the generated .eslintrc.js to:
  parserOptions: {
    parser: '@typescript-eslint/parser',
    project: `${__dirname}/tsconfig.json`,
    extraFileExtensions: [".vue"]
  }
  • Run ESLint on App.vue

My mistake earlier was forgetting the extraFileExtensions property. Without that, we get the new error. With it, it lints properly. It even correctly parses the components/HelloWorld.vue file when creating the project for linting... I was surprised that that worked, but I'm happy that it did. :)

@bradzacher
Copy link
Member

Just to confirm - did you try that on a SFC (Single File Component)? (eg below)

Based on #404 I wasn't sure if it'd work.
If so then that's great! We can mark that as closed.

Just for sanity can you add a test like this, just so we can make sure that we don't break support again.
(I've never used vue, but I'm pretty sure that this is valid vue code):

// .eslint.js
module.exports = {
  parser: 'vue-eslint-parser',
  parserOptions: {
    "parser": '@typescript-eslint/parser',
    "project": `${__dirname}/tsconfig.json`,
    extraFileExtensions: ['.vue'],
  },
  plugins: ['@typescript-eslint'],
  rules: {
    '@typescript-eslint/no-implicit-any': 'error',
  },
};
<!-- Hello.vue -->
<template>
    <div>
        <div>
		    <!-- !!!!! expected error !!!!! -->
			Hello {{ (name as any) }}{{exclamationMarks}}
		</div>
        <button @click="decrement">-</button>
        <button @click="increment">+</button>
    </div>
</template>

<script lang="ts">
import Vue from "vue";
export default Vue.extend({
    props: ['name', 'initialEnthusiasm'],
    data() {
        return {
            enthusiasm: this.initialEnthusiasm,
        }
    },
    methods: {
        increment() { this.enthusiasm++; },
        decrement() {
            if (this.enthusiasm > 1) {
                this.enthusiasm--;
            }
        },
    },
    computed: {
        exclamationMarks(): any { // !!!!! expected error !!!!!
            return Array(this.enthusiasm + 1).join('!');
        }
    }
});
</script>

@bradzacher bradzacher mentioned this pull request Jul 27, 2019
14 tasks
@uniqueiniquity
Copy link
Contributor Author

Yep, I can lint that file fine (assuming you meant no-explicit-any).
One thing to point out is that only the any in the script tag is flagged.
In order to traverse the template, the rule needs to use the context.parserServices.defineTemplateBodyVisitor, and since ours isn't vue-specific, we don't traverse the expression containing the other any.

Is the best way to add that test via another integration test with docker?
I'm happy to do it, but I don't really know anything about docker so it might take me a bit :)

@bradzacher
Copy link
Member

Awesome, it's good that it works!

Is the best way to add that test via another integration test with docker?

That's a good question for @JamesHenry - I don't know that test harness at all.


only the any in the script tag is flagged
context.parserServices.defineTemplateBodyVisitor

I didn't know about that function ...
Do we want to add defineTemplateBodyVisitor (in a separate PR)?

@JamesHenry JamesHenry merged commit 3777b77 into typescript-eslint:master Jul 28, 2019
@JamesHenry
Copy link
Member

I'll add the integration test in a follow up, thanks again @uniqueiniquity!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
breaking change This change will require a new major version to be released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multi-project configuration doesn't type check files not included in any project
3 participants