Skip to content

Conversation

@btmills
Copy link
Member

@btmills btmills commented Jul 28, 2020

#153 and #152 should be merged first.

@btmills btmills force-pushed the example-typescript branch from 6b72d5c to 724c8c9 Compare August 1, 2020 20:48
@btmills btmills changed the base branch from example-react to master August 1, 2020 20:50
@btmills btmills changed the base branch from master to example-react August 1, 2020 20:52
@btmills btmills force-pushed the example-react branch 2 times, most recently from c96f269 to ff6ffe1 Compare August 1, 2020 22:06
@btmills btmills force-pushed the example-typescript branch from 724c8c9 to 636b706 Compare August 1, 2020 22:06
@btmills btmills force-pushed the example-typescript branch from 636b706 to 890f7fd Compare August 1, 2020 22:11
@btmills btmills force-pushed the example-typescript branch from 890f7fd to 6018276 Compare August 1, 2020 22:27
Copy link
Member

@kaicataldo kaicataldo left a comment

Choose a reason for hiding this comment

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

This generally looks good to me! Should we add a tsconfig.json file in this example?

Base automatically changed from example-react to master August 2, 2020 20:43
btmills added 2 commits August 2, 2020 16:49
It works on the first block but not the second. See diff for additional
context. Any ideas?
@btmills btmills force-pushed the example-typescript branch from 6018276 to 1c78364 Compare August 2, 2020 22:12
@btmills
Copy link
Member Author

btmills commented Aug 2, 2020

@kaicataldo today I learned a bit about how type-aware linting works! Unfortunately I can't get it fully working. I can get the first ts code block in README.md to lint, but the second code block reports a configuration error even though I tried to configure it the same way. I left some comments in 1c78364. Any ideas?

@btmills
Copy link
Member Author

btmills commented Aug 10, 2020

@kaicataldo I tried a few more things and still wasn't able to get type-aware linting to work outside the first code block. Unless you have any ideas, what do you think of adding a note to this saying non-type-aware rules work with TS but type-aware rules don't for now?

@kaicataldo
Copy link
Member

That works for me! I looked at it for a bit and wasn't able to figure it out either. Maybe @bradzacher has an idea?

kaicataldo
kaicataldo previously approved these changes Aug 10, 2020
@bradzacher
Copy link

bradzacher commented Aug 10, 2020

I don't believe type-aware linting will work. TL;DR - type-aware linting relies upon a file on disk, but preprocessors produce "fake" files.


Long answer:
typescript-eslint is built on top of typescript itself. For standard linting, TS has APIs which let as parse raw text (and we then convert the TS AST to an ESTree AST).

For type-aware linting, we have to leverage TS's heavy compiler API, which works around having a tsconfig which references files on disk.

ESLint preprocessors are a bit of a hack in that a preprocessor essentially splits one file into many "fake" files that don't exist on disk. Because there's no file on disk, the TS API can't find the file specified in its data structures, so we throw an error.

@btmills
Copy link
Member Author

btmills commented Aug 13, 2020

Thank you @bradzacher, that's very helpful! Knowing that, I'm curious how type-aware rules worked at all in the first code block in 1c78364. Gremlins, apparently.

In da4b9b0, I removed tsconfig.json and the configuration for type-aware linting and added a brief note to the example's readme. With that, I think this is ready. GitHub auto-dismissed @kaicataldo's approval of the plan that I implemented in the commit, so I'll hold off merging for a couple days in case someone has time to drop by and give it a 👍

Copy link
Member

@kaicataldo kaicataldo left a comment

Choose a reason for hiding this comment

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

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants