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 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 added 2 commits Oct 19, 2019
@bradzacher bradzacher marked this pull request as ready for review Oct 19, 2019
@codecov

This comment has been minimized.

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
Copy link
Member

JamesHenry left a comment

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

@bradzacher bradzacher merged commit ed5564d into master Oct 19, 2019
6 of 7 checks passed
6 of 7 checks passed
codecov/patch 89.92% of diff hit (target 90%)
Details
Semantic Pull Request ready to be squashed
Details
codecov/project 94.09% (+0.06%) compared to 0c85ac3
Details
typescript-eslint.typescript-eslint Build #20191019.5 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 rfc-tsconfig-invalidation branch Oct 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.