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

fix: support long running "watch" lint sessions #973

Merged
merged 9 commits into from Oct 12, 2019

Conversation

@bradzacher
Copy link
Member

bradzacher commented Sep 13, 2019

Fixes #864

This PR adds support for watching directories and configuration files so that we can force typescript to recalculate the program if new files are added.

In this PR I also setup typescript project references for the repo.
This was because I was jumping back and forth between packages to test the linting and watching, and I was annoyed at having to ensure I'd built everything in the dep tree.

There is one very, very minor problem with this that I don't know how to solve right now.
In an IDE, there is a race between when the file is opened and the eslint plugin lints it, and when the ts program is updated.

This means that if the editor is particularly quick to open the file (<250ms), then the initial lint of the empty file will throw the "file not in project" parsing error.

However I figured out this doesn't last long. If you start typing, then the plugin will re-lint the file (which now exists in the ts program), and the parser error will disappear. Or you can close and reopen the file.

Video showing it off working in VSCode: https://youtu.be/0My26ejZJVk

@bradzacher bradzacher added the bug label Sep 13, 2019
@bradzacher bradzacher requested review from JamesHenry and uniqueiniquity Sep 13, 2019
@typescript-eslint

This comment was marked as resolved.

Copy link

typescript-eslint bot commented Sep 13, 2019

Thanks for the PR, @bradzacher!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!


🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

@uniqueiniquity

This comment has been minimized.

Copy link
Contributor

uniqueiniquity commented Sep 13, 2019

I really like this approach... but as you mentioned, you're hitting the same issue that I initially did, where the process won't exit because it's holding onto the file watchers. Sounds like the cleanup function would be great if we can get it.

@bradzacher

This comment has been minimized.

Copy link
Member Author

bradzacher commented Sep 13, 2019

I tried to do something tricky, but it didn't seem to work:

  • store the watchDirectory callback
  • when an unknown file is encountered, call the callback to update the program with it
  • get new file from program

When you call the watchDirectory callback, typescript schedules an the update for 250ms later (so it doesn't trigger multiple updates for a "save all files" cycle from an IDE).

Because the parser isn't asynchronous, we can't wait for this time... grr


I did some research into handles in NodeJS.

I don't see a way to create "weak" handles (i.e. handles which won't block the process).
Also, I couldn't see a way to have node inform you that you're the one keeping the process alive.


So I guess that either we get eslint to better support stateful parsers, or we create our own CLI wrapper.

@ark120202

This comment has been minimized.

Copy link

ark120202 commented Sep 13, 2019

fs.watch and fs.watchFile (which are used by some of TypeScript watch strategies) have a persistent option, which when is set to false doesn't block process from exiting, but I think that still would have synchronization issues.

Another option is to have a virtual program host (fork-ts-checker-webpack-plugin pull request) or a language service host (ts-node) that would use actual file content provided by eslint (which also can give better support for virtual TypeScript files, like in Vue) and when file is linted again just update it. Though it still may require some changes in eslint since it would need to have all files for the first lint, but maybe there are workarounds.

@bradzacher

This comment has been minimized.

Copy link
Member Author

bradzacher commented Sep 13, 2019

fs.watch and fs.watchFile have a persistent option...

Interesting.
Maybe this PR would work if I switched from using ts.sys.watchDirectory to instead manually using fs.watch...
HMM thank you for this, I missed this when reading the docs last night. I was grepping for different keywords than "persistent".

I'll investigate this more tonight!


when file is linted again just update it

We do do that already. We treat eslint as the source of truth for file contents - to the point that we don't even attach file system watchers for any of the js/ts files being linted!

The problem is that when in watch mode, new files can be introduced. These files are do not exist in the ts program that's already been constructed.

So we need a way to tell when we need to tell typescript to update the program. This is where watching the directories (and watching the tsconfigs) comes in:

  • watch the directory and we'll be notified when a new file is added (AND watch the tsconfig so we can tell when the config changes).
  • tell TS to update the program.
  • ESLint then asks us to lint the new file - by this point the program is already updated, and the new file exists!

I did think about an approach that involves us manually checking an unknown file against the provided tsconfigs to determine which programs to recalculate, but there are two problems with this:

  1. I don't know if (/ don't think) TS exposes the logic for determining if a file is included within a given tsconfig. If this isn't exposed:
    • We'd have to write our own logic for it - bad because we'd have to maintain it and keep it synced with typescript, or
    • PR the typescript repo to expose it - bad because it then hard ties us to a recent TS version. A large portion of our users are behind by a heap of TS versions, so this makes PRing it a non-starter.
  2. (this is the biggest blocker) Typescript schedules program updates for 250ms after being told about a filesystem update.
    • ESLint parsers live within an entirely synchronous world.
      • I believe that we're the most complex eslint parser in existence by a long, long, long way; hence the parser API is so simple.
    • There is currently no mechanism to tell eslint to wait for the file's contents.
    • Could defs RFC eslint and get that added, though there may be a better approach (like RFCing eslint to add better support for stateful parsers / parsers with a lifecycle).
bradzacher added 5 commits Sep 14, 2019
# Conflicts:
#	packages/typescript-estree/package.json
@codecov

This comment has been minimized.

Copy link

codecov bot commented Sep 16, 2019

Codecov Report

Merging #973 into master will decrease coverage by 0.16%.
The diff coverage is 70.9%.

@@            Coverage Diff             @@
##           master     #973      +/-   ##
==========================================
- Coverage   94.18%   94.01%   -0.17%     
==========================================
  Files         115      115              
  Lines        5022     5064      +42     
  Branches     1406     1416      +10     
==========================================
+ Hits         4730     4761      +31     
- Misses        165      175      +10     
- Partials      127      128       +1
Impacted Files Coverage Δ
...ges/experimental-utils/src/ts-eslint/RuleTester.ts 0% <0%> (ø) ⬆️
packages/parser/src/parser.ts 100% <100%> (ø) ⬆️
packages/typescript-estree/src/parser.ts 91.89% <57.14%> (-1.17%) ⬇️
packages/typescript-estree/src/tsconfig-parser.ts 86.91% <80.95%> (-0.93%) ⬇️
@bradzacher bradzacher marked this pull request as ready for review Sep 16, 2019
@bradzacher

This comment has been minimized.

Copy link
Member Author

bradzacher commented Sep 16, 2019

cc @JamesHenry and @uniqueiniquity

Thanks to the great suggestion by @ark120202 - I've gotten this working using chokidar to do the file watching.

This is ready for review!

package.json Show resolved Hide resolved
.eslintrc.js Show resolved Hide resolved
packages/parser/src/parser.ts Show resolved Hide resolved
packages/typescript-estree/src/parser.ts Show resolved Hide resolved
packages/typescript-estree/src/parser.ts Show resolved Hide resolved
bradzacher added 2 commits Oct 8, 2019
Copy link
Member

JamesHenry left a comment

Thanks for working on this!

@bradzacher bradzacher changed the title fix(typescript-estree): support long running "watch" lint sessions fix: support long running "watch" lint sessions Oct 12, 2019
@bradzacher bradzacher merged commit 854620e into master Oct 12, 2019
6 of 7 checks passed
6 of 7 checks passed
codecov/patch 70.9% of diff hit (target 90%)
Details
Semantic Pull Request ready to be squashed
Details
codecov/project 94.01% (-0.17%) compared to fec73b0
Details
typescript-eslint.typescript-eslint Build #20191012.8 succeeded
Details
typescript-eslint.typescript-eslint (Primary code validation and tests) Primary code validation and tests succeeded
Details
typescript-eslint.typescript-eslint (Run unit tests on other Node.js versions node_10_x) Run unit tests on other Node.js versions node_10_x succeeded
Details
typescript-eslint.typescript-eslint (Run unit tests on other Node.js versions node_8_x) Run unit tests on other Node.js versions node_8_x succeeded
Details
@bradzacher bradzacher deleted the fix-watch branch Oct 12, 2019
@skovhus

This comment has been minimized.

Copy link

skovhus commented Oct 14, 2019

I’m wondering what the difference between this approach and the workaround mentioned here: #864 (comment)

@JamesHenry

This comment has been minimized.

Copy link
Member

JamesHenry commented Oct 14, 2019

Good call out @skovhus, I've added an additional comment there: #864 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.