Watch for the creation of missing files by amcasey · Pull Request #16684 · microsoft/TypeScript

amcasey · 2017-06-21T18:04:32Z

When we discover that a file is missing, call watchFile on its path so that we can update our state when it is created.

For tsc --watch, we schedule a new build
For tsserver, we update the containing projects

Caveat: VS will not know about the state change in tsserver until the next time it synchronizes its project list

msftclas · 2017-06-21T18:04:37Z

@amcasey,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

amcasey · 2017-06-21T18:04:47Z

This PR supersedes #16527.

amcasey · 2017-06-21T19:42:47Z

+        /**
+         * Get a list of file names that were passed to 'createProgram' or referenced in a
+         * program source file but could not be located.
+         */


This doesn't need to be optional.

ghost · 2017-06-23T17:47:57Z

+            // will be created until we encounter a change that prevents complete structure reuse.
+            // During this interval, creation of the file will go unnoticed.  We expect this to be
+            // both rare and low-impact.
+            const missingFilePaths: Path[] = oldProgram.getMissingFilePaths() || emptyArray;


It looks like getMissingFilePaths already never returns undefined. (Let's keep it that way.) Inline this declaration.

ghost · 2017-06-23T17:48:19Z

+            }
+
+            for (const p of oldProgram.getMissingFilePaths()) {
+                filesByName.set(p, undefined);


The type of filesByName should indicate that this can be undefined.

I'm happy to update the type, but I should mention that the map already had undefined values (this change just ensures that they persist across structure reuse).

To confirm, you're suggesting const filesByName = createFileMap<SourceFile | undefined>(); ?

That doesn't appear to have any effect on the inferred type. Presumably, it would matter with strict null-checking enabled?

Yes, it's just documentation for now.

ghost · 2017-06-23T17:49:06Z

@@ -340,11 +340,20 @@ namespace ts {
                    }

                    function fileChanged(curr: any, prev: any) {


It was this way before, but it would be nice to remove any and include a type declaration saying what you expect this to be.

I would be interested to learn how to do this. Do I just specify that it has to have an mtime property or is there something more?

After getting offline help, I don't think this is worth fixing as part of this change.

ghost · 2017-06-23T17:51:37Z

+            const missingPaths = compileResult.program.getMissingFilePaths() || [];
+            missingPaths.forEach((path: Path): void => {
+                const fileWatcher = sys.watchFile(path, (_fileName: string, removed?: boolean) => {
+                    // removed = deleted ? true : (added ? false : undefined)


This could use an enum rather than true | false | undefined. true -> E.Deleted, false -> E.Added, undefined -> E.Changed.

I was under the impression that we didn't control the signature, but I'll confirm.

ghost · 2017-06-23T17:52:15Z

+        /**
+         * Get a list of file names that were passed to 'createProgram' or referenced in a
+         * program source file but could not be located.
+         */


Would make this internal. (/* @internal */)

ghost · 2017-06-23T17:53:36Z

+        const referenceFile: Harness.Compiler.TestFile = {
+            unitName: referenceFileName,
+            content:
+                "/// <reference path=\"d:/imaginary/nonexistent1.ts\"/>\n" + // Absolute


Is it necessary to test all the different kinds of paths here? Also, would include a reference to an existing file for contrast.

I believe I have a test that mixes existent and non-existent paths, but I'll confirm.

I do have such a test, but it is for root files. I thought that duplicating all of the tests for triple-slash references would be redundant (and add to maintenance and runtime costs), so I skipped them. Let me know if you feel strongly and I'll make a second set of tests.

No, should be fine then.

ghost · 2017-06-23T17:54:43Z

+        it("handles missing root file", () => {
+            const program = createProgram(["./nonexistent.ts"], options, testCompilerHost);
+            const missing = program.getMissingFilePaths();
+            assert.isDefined(missing);


Use assert.deepEqual instead of asserting the length and elements separately.

No need for assert.isDefined since you're about to assert that it's equal to a defined value.

ghost · 2017-06-23T17:58:25Z

        private rootFilesMap: FileMap<ScriptInfo> = createFileMap<ScriptInfo>();
        private program: ts.Program;
        private externalFiles: SortedReadonlyArray<string>;
+        private missingFilesMap: FileMap<FileWatcher> = createFileMap<FileWatcher>();


Don't need a FileMap, just a Map. Then you can iterate using forEach. (#16715)

Don't I need a FileMap if the key type is Path?

A Path is a subtype of string, so you can use those as map keys. (It might be slightly more type safe to specify that keys must be Paths, but I don't think that's worth using a FileMap.)

I don't understand when we use Path and FileMap. Do you have some time to explain?

Also, I've made the change.

ghost · 2017-06-23T18:12:04Z

+            // Clean up file watchers waiting for missing files
+            for (const p of this.missingFilesMap.getKeys()) {
+                this.missingFilesMap.get(p).close();
+                this.missingFilesMap.remove(p);


What's the point of removing items one-by-one if you just set this.missingFileMap = undefined; at the end?

Good point. I cleared the map as an afterthought.

ghost · 2017-06-23T18:15:19Z

                }
            }

+            if (hasChanges) {


It was this way before, but I would prefer const hasChanges = ...; over `let hasChanges = false; if (...) { hasChanges = true; }

ghost · 2017-06-23T18:15:51Z

            }

+            if (hasChanges) {
+                const missingFilePaths = this.program.getMissingFilePaths() || emptyArray;


Again, getMissingFilePaths should always be defined, so no need for || emptyArray.

ghost · 2017-06-23T18:25:55Z


+            if (hasChanges) {
+                const missingFilePaths = this.program.getMissingFilePaths() || emptyArray;
+                const missingFilePathsSet = createMap<true>();


arrayToMap(missingFilePaths, path => path);

Does that have an advantage other than brevity?

No, it should have an identical result.

It seems to result in a String-Path map, rather than a Path-true map.

There should probably be a helper for this:

export function arrayToSet(array: string[]): Map<boolean> { return arrayToMap(array, key => key, () => true); }

ghost · 2017-06-23T18:30:07Z

    export function getDocumentHighlights(program: Program, cancellationToken: CancellationToken, sourceFile: SourceFile, position: number, sourceFilesToSearch: SourceFile[]): DocumentHighlights[] | undefined {
        const node = getTouchingWord(sourceFile, position, /*includeJsDocComment*/ true);
-        if (!node) return undefined;
+        // Note that getTouchingWord indicates failure by returning the sourceFile node.


Could this be its own PR?

It's a separate commit, so it would be no extra work to make it a separate PR. What's the local convention?

Ask around?

I didn't find strong support for pulling it out. If it were more contentious, I would.

Was there a test that was breaking under the old behavior?

No, otherwise the code would have been correct. 😉

ghost · 2017-06-23T18:31:23Z

+                });
+
+                // Missing files that are not yet watched should be added to the map.
+                missingFilePaths.forEach(p => {


Use a memorable variable name for p, like missingPath.

I'm not in the habit of giving detailed names to iteration variables, but sure.

amcasey · 2017-06-27T21:52:58Z

@Andy-MS I think I've addressed all of your feedback, other than the ones marked with ❓.

ghost · 2017-06-28T14:27:14Z

+     *
+     * @param array the array of input elements.
+     *
+     * This function makes no effort to avoid collisions; if any two elements produce


This is a Map<true>, so the elements of the array aren't present in the result. So the element with the higher index in the array will be the one associated with the produced key can't be true.

ghost · 2017-06-28T14:29:45Z

                // Files that are no longer missing (e.g. because they are no longer required)
                // should no longer be watched.
-                for (const missingFilePath of this.missingFilesMap.getKeys())
+                const missingFilesToDelete: string[] = [];


It should be safe to delete values from a map while iterating over it. Are you sure this was the cause of an error?

No, I just couldn't find any documentation indicating that it was safe.

amcasey · 2017-06-28T18:25:34Z

After a high-level review from @mhegazy and a detailed review from @Andy-MS, I think this is ready to be merged. Any final comments? (cc: @rbuckton)

ghost · 2017-06-28T20:04:28Z

            }
        }

+        const missingFilePaths = filesByName.getKeys().filter(p => !filesByName.get(p));


This will have a semantic conflict with #16724 as filesByName will be a Map, which doesn't have a getKeys(). You can use mapDefined<[string, string], string>(arrayFrom(m.entries()), ([path, value]) => value === undefined ? undefined : path);. Or add a version of mapDefined that works on iterators to avoid the arrayFrom.

I'll rebase on top of your commit.

ghost · 2017-06-28T20:05:26Z

+            // will be created until we encounter a change that prevents complete structure reuse.
+            // During this interval, creation of the file will go unnoticed.  We expect this to be
+            // both rare and low-impact.
+            for (const missingFilePath of oldProgram.getMissingFilePaths()) {


Nit: just use oldProgram.getMissingFilePaths().some(path => host.fileExists(path))

ghost · 2017-06-28T20:06:26Z

+                        const created = !isCurrZero && isPrevZero;
+                        const deleted = isCurrZero && !isPrevZero;
+
+                        const eventKind = created


Nit: No need for created and deleted variables, just use eventKind === FileWatcherEventKind.Changed instead of !created && !deleted

I think the existence of the constants makes the code more readable, but I agree that the if is better expressed in terms of eventKind.

ghost · 2017-06-28T20:06:57Z

            reportWatchDiagnostic(createCompilerDiagnostic(Diagnostics.Compilation_complete_Watching_for_file_changes));
+
+            const missingPaths = compileResult.program.getMissingFilePaths();
+            missingPaths.forEach((path: Path): void => {


Nit: Avoid annotating the types of callbacks if not necessary.

ghost · 2017-06-28T20:08:37Z

                return;
            }

+            // removed = deleted ? true : (added ? false : undefined)


comment outdated

Oops, missed one.

`getTouchingWord` indicates failure by returning the sourceFile node, rather than `undefined`.

1. Expose missing files from the `Program`. 2. In `tsc --watch` and `tsserver`, add file watchers to missing files. 3. When missing files are created, schedule compilation (tsc) or refresh the containing projects (tsserver).

Call `this.projectService.host.watchFile`, rather than `ts.sys.watchFile` so that it gets mocked correctly in the unit tests. Repair two failing tests.

1. Test `Program.getMissingFilePaths` 2. Test program structure reuse (i.e. that the appearance of a missing file prevents complete reuse)

Make Program.getMissingFilePaths required Assume getMissingFilePaths always returns a defined value Make getMissingFilePaths internal Replace nullable-bool with enum Update type to reflect possibility of undefined Use deepEqual to simplify tests Make condition const Don't bother cleaning up map before freeing it Switch from foreach to for-of to simplify debugging Use a Map, rather than a FileMap, to track open FileWatchers Fix compilation errors Introduce and consume arrayToSet Fix lint warnings about misplaced braces Delete incorrect comment Delete from map during iteration Eliminate unnecessary type annotations

ghost · 2017-06-30T17:35:01Z

+                // Missing files that are not yet watched should be added to the map.
+                for (const missingFilePath of missingFilePaths) {
+                    if (!this.missingFilesMap.has(missingFilePath)) {
+                        const fileWatcher = this.projectService.host.watchFile(missingFilePath, (_filename: string, eventKind: FileWatcherEventKind) => {


host.watchFile is actually optional. We could make it non-optional and mark it as a breaking change. See also #16776.

mihailik · 2017-06-30T07:31:41Z

+     *
+     * @param array the array of input elements.
+     */
+    export function arrayToSet<T>(array: T[], makeKey: (value: T) => string): Map<true> {


This is used exactly ONCE. Is it worth extracting like that -- from performance, code clarity or any other perspective?

It's generally useful to factor generic code out of a more specific context. That way you don't have to worry about the specifics of how an array is converted to a set, and only have to worry about the actual problem domain.

amcasey requested review from a user, rbuckton and sheetalkamat June 21, 2017 18:04

msftclas added the cla-not-required label Jun 21, 2017

amcasey mentioned this pull request Jun 21, 2017

Watch for the creation of missing files #16527

Closed

amcasey changed the title ~~Vsts434619~~ Watch for the creation of missing files Jun 21, 2017

amcasey commented Jun 21, 2017

View reviewed changes

ghost reviewed Jun 23, 2017

View reviewed changes

ghost reviewed Jun 28, 2017

View reviewed changes

amcasey added 7 commits June 29, 2017 10:39

Update error case check

587309d

`getTouchingWord` indicates failure by returning the sourceFile node, rather than `undefined`.

Track missing files

4863ada

1. Expose missing files from the `Program`. 2. In `tsc --watch` and `tsserver`, add file watchers to missing files. 3. When missing files are created, schedule compilation (tsc) or refresh the containing projects (tsserver).

Clean up file watchers on project close

a39e969

Confirm method is defined before calling

4652fc4

Watch files through the host

6d200bf

Call `this.projectService.host.watchFile`, rather than `ts.sys.watchFile` so that it gets mocked correctly in the unit tests. Repair two failing tests.

Add missing file unit tests

0f683ac

1. Test `Program.getMissingFilePaths` 2. Test program structure reuse (i.e. that the appearance of a missing file prevents complete reuse)

amcasey force-pushed the Vsts434619 branch from 62d5645 to 569ecab Compare June 29, 2017 18:30

amcasey merged commit ac72803 into microsoft:master Jun 29, 2017

amcasey deleted the Vsts434619 branch June 29, 2017 20:41

ghost reviewed Jun 30, 2017

View reviewed changes

mihailik reviewed Jun 30, 2017

View reviewed changes

microsoft locked and limited conversation to collaborators Jun 14, 2018

		@@ -340,11 +340,20 @@ namespace ts {
		}

		function fileChanged(curr: any, prev: any) {

Conversation

amcasey commented Jun 21, 2017

Uh oh!

msftclas commented Jun 21, 2017

Uh oh!

amcasey commented Jun 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Jun 23, 2017 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ghost Jun 23, 2017 •

edited by ghost

Loading