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): support long running lint without watch #1106

Merged
merged 5 commits into from
Oct 19, 2019

Conversation

bradzacher
Copy link
Member

@bradzacher bradzacher commented Oct 19, 2019

Fixes #1079
Fixes #1080
Fixes #1084
Fixes #1091
Fixes #1107

This PR does one main thing: it adds support for handling file updates without watchers.
It took the better part of an 8 hour session, but I figured it out.
I didn't even have to drop out of watch programs to do it.

There is a bit of noise on this PR, and I'm sorry for that. The important changes are on fa963c8.
f28e4dd was a quick refactor I did to help me look at the problem properly - all I did was move the various program creation functions we have into their own folders.

The solution in a nutshell.

  1. The realisation that I can override the setTimeout function that the watch program uses. This let me turn the async 250ms queued updates into synchronous updates, which solved the huge problem with how it handled directory updates.
    • /*
      * The watch change callbacks TS provides us all have a 250ms delay before firing
      * https://github.com/microsoft/TypeScript/blob/b845800bdfcc81c8c72e2ac6fdc2c1df0cdab6f9/src/compiler/watch.ts#L1013
      *
      * We live in a synchronous world, so we can't wait for that.
      * This is a bit of a hack, but it lets us immediately force updates when we detect a tsconfig or directory change
      */
      const oldSetTimeout = watchCompilerHost.setTimeout;
      watchCompilerHost.setTimeout = (cb, ms, ...args): unknown => {
      if (ms === 250) {
      cb();
      return null;
      }
      return oldSetTimeout && oldSetTimeout(cb, ms, ...args);
      };
  2. Using fs.stat to check for tsconfig changes instead of watching for changes.
    • const tsconfigStatTimestamp = new Map<string, number>();
      function hasTSConfigChanged(tsconfigPath: string): boolean {
      const stat = fs.statSync(tsconfigPath);
      const lastModifiedAt = stat.mtimeMs;
      const cachedLastModifiedAt = tsconfigStatTimestamp.get(tsconfigPath);
      tsconfigStatTimestamp.set(tsconfigPath, lastModifiedAt);
      if (cachedLastModifiedAt === undefined) {
      return false;
      }
      return Math.abs(cachedLastModifiedAt - lastModifiedAt) > Number.EPSILON;
      }
    • if (hasTSConfigChanged(tsconfigPath)) {
      /*
      * If the stat of the tsconfig has changed, that could mean the include/exclude/files lists has changed
      * We need to make sure typescript knows this so it can update appropriately
      */
      log('tsconfig has changed - triggering program update. %s', tsconfigPath);
      fileWatchCallbackTrackingMap
      .get(tsconfigPath)!
      .forEach(cb => cb(tsconfigPath, ts.FileWatcherEventKind.Changed));
      }
  3. Manually triggering a directory update if the file isn't in the folder
    • /*
      * Missing source file means our program's folder structure might be out of date.
      * So we need to tell typescript it needs to update the correct folder.
      */
      log('File was not found in program - triggering folder update. %s', filePath);
      // Find the correct directory callback by climbing the folder tree
      let current: string | null = null;
      let next: string | null = path.dirname(filePath);
      let hasCallback = false;
      while (current !== next) {
      current = next;
      const folderWatchCallbacks = folderWatchCallbackTrackingMap.get(current);
      if (folderWatchCallbacks) {
      folderWatchCallbacks.forEach(cb =>
      cb(current!, ts.FileWatcherEventKind.Changed),
      );
      hasCallback = true;
      break;
      }
      next = path.dirname(current);
      }
      if (!hasCallback) {
      /*
      * No callback means the paths don't matchup - so no point returning any program
      * this will signal to the caller to skip this program
      */
      log('No callback found for file, not part of this program. %s', filePath);
      return null;
      }
  4. Manually triggering file deletions to support file renaming. This one feels like a bug in the watch program. It assumes if the number of files in the program doesn't change, then the program hasn't changed. If you rename a file, the number of files in the program doesn't change, but the list ofc doesn't match the new list of files reported. I think the idea is that it's expecting a file watcher to signal a deletion when a file is renamed. Regardless, this is a last resort to manually trigger file deletions when it thinks the file is renamed.
    • /*
      * At this point we're in one of two states:
      * - The file isn't supposed to be in this program due to exclusions
      * - The file is new, and was renamed from an old, included filename
      *
      * For the latter case, we need to tell typescript that the old filename is now deleted
      */
      log(
      'File was still not found in program after directory update - checking file deletions. %s',
      filePath,
      );
      const rootFilenames = updatedProgram.getRootFileNames();
      // use find because we only need to "delete" one file to cause typescript to do a full resync
      const deletedFile = rootFilenames.find(file => !fs.existsSync(file));
      if (!deletedFile) {
      // There are no deleted files, so it must be the former case of the file not belonging to this program
      return null;
      }
      const fileWatchCallbacks = fileWatchCallbackTrackingMap.get(deletedFile);
      if (!fileWatchCallbacks) {
      // shouldn't happen, but just in case
      log('Could not find watch callbacks for root file. %s', deletedFile);
      return updatedProgram;
      }
      log('Marking file as deleted. %s', deletedFile);
      fileWatchCallbacks.forEach(cb =>
      cb(deletedFile, ts.FileWatcherEventKind.Deleted),
      );

If my assumptions are correct, the cost of all of this should only be paid in watch mode, when a new file is encountered.

Other notes:

  • Refactored to isolate all the program creation functions
  • Added more logging calls (it was easier to use logging than always attempting to attach a debugger). I left them in because I figured it might help us in future.
  • Added (slightly) more complex unit tests to ensure both deeply nested file creation, and cross folder file renaming actually works.
  • Fixed a bug in the rule tester with it not respecting locally defined test case filenames (it only used the filename from the top level, which broke tests).
  • Stopped passing both options and extra into the functions by adding filePath to extra.

Video of it working perfectly in vscode: https://youtu.be/sg54KuUdw9U

@bradzacher bradzacher marked this pull request as ready for review October 19, 2019 09:08
@codecov
Copy link

codecov bot commented Oct 19, 2019

Codecov Report

Merging #1106 into master will increase coverage by 0.06%.
The diff coverage is 89.92%.

@@            Coverage Diff             @@
##           master    #1106      +/-   ##
==========================================
+ Coverage   94.02%   94.09%   +0.06%     
==========================================
  Files         115      120       +5     
  Lines        5123     5200      +77     
  Branches     1434     1442       +8     
==========================================
+ Hits         4817     4893      +76     
+ Misses        177      176       -1     
- Partials      129      131       +2
Impacted Files Coverage Δ
packages/typescript-estree/src/parser.ts 95.57% <100%> (+3.36%) ⬆️
...ript-estree/src/create-program/createSourceFile.ts 100% <100%> (ø)
...estree/src/create-program/createIsolatedProgram.ts 73.91% <73.91%> (ø)
...-estree/src/create-program/createDefaultProgram.ts 76.19% <76.19%> (ø)
...ges/typescript-estree/src/create-program/shared.ts 83.33% <83.33%> (ø)
...-estree/src/create-program/createProjectProgram.ts 88.88% <88.88%> (ø)
...pt-estree/src/create-program/createWatchProgram.ts 92.99% <92.99%> (ø)
... and 3 more

JamesHenry
JamesHenry previously approved these changes Oct 19, 2019
Copy link
Member

@JamesHenry JamesHenry left a comment

Choose a reason for hiding this comment

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

Seems really promising, thanks a lot for all the effort that went into this!

@bradzacher bradzacher added the DO NOT MERGE PRs which should not be merged yet label Oct 19, 2019
@fubar
Copy link

fubar commented Nov 20, 2019

@bradzacher thanks for all your work on this! I hadn't gotten back to this until now and it works like a charm in IntelliJ. Now I don't have to keep toggling my eslint integration off and on to pick up new and renamed files anymore. Much appreciated!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
3 participants