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

Handle file versioning little better in tsc --watch mode #21693

Merged
merged 4 commits into from
Feb 7, 2018

Conversation

sheetalkamat
Copy link
Member

Root cause:
Earlier the source file cache use to either have { version, sourceFile, fileWatcher } or string.
string meant file is on present on the disk and we wont go ahead and create source file at all for these whenever asked for new source file.
During watch callback, especially during wild card directory callback, if the file wasnt present on the disk, we would just remove the entry from cache.. So in below sequence the file version didnt change even though the content had changed:

  1. Watch created, file was on disk, so version is set to inital version = 1
  2. File deleted, deleted from the cache
  3. File get created
  4. Updating the program structure, since file is on the disk, create source file and set the version as initial version (as the entry wasn't there in the cache) = 1
  5. During emit since old program and new program had files with same version, they arent marked as changed and hence not emitted

Fix:
Now whenever the file callback is invoked, and cache has file as missing, instead of removing the entry, we would create a temp entry that represents that file may be on the disk = { version++ }
This ensures that next time program is able to find the file on the disk and create new source file, it wont have initial version. Thus when creating new source file from oldProgram, if the file is changed, it would always have different version than the oldSourceFile from oldProgram.

Fixes #21444

This ensures that when file is deleted and re-created, the file version isnt same to ensure emits correctly
Fixes #21444
@sheetalkamat sheetalkamat force-pushed the fileChangeThroughDeleteAndCreate branch from 32b0352 to 2684784 Compare February 6, 2018 20:30
resolutionCache.invalidateResolutionOfFile(path);
if (isFileMissingOnHost(hostSourceFile)) {
// The next version, lets set it as presence unknown file
sourceFilesCache.set(path, { version: Number(hostSourceFile) + 1 });
Copy link
Contributor

Choose a reason for hiding this comment

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

can we make it a number instead of a string since we are using it as a version?

Copy link
Member Author

@sheetalkamat sheetalkamat Feb 6, 2018

Choose a reason for hiding this comment

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

Done. Earlier i was wondering if i should do that given we do checks like if (hostSourceFile). But now that we have initial version starting at 1 makes it easier to do that switch

Copy link
Contributor

Choose a reason for hiding this comment

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

so we do not need the call to Number?

@@ -721,10 +744,10 @@ namespace ts {
// there was version update and new source file was created.
if (hostSourceFileInfo) {
// record the missing file paths so they can be removed later if watchers arent tracking them
if (isString(hostSourceFileInfo)) {
if (isFileMissingOnHost(hostSourceFileInfo)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

these functions should return a user-defined type guards to avoid the casts below.

Copy link
Member Author

Choose a reason for hiding this comment

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

In the line below the hostSourceFile could be FilePresenceUnknownOnHost or FileMayBePresentOnHost . It is kind of redundant to check if isFilePresentOnHost since it only checks if sourceFile is present on the host and the next check is going to check if sourceFile === oldSourceFile (oldSourceFile is never undefined). Hence the cast

@JulioJu
Copy link

JulioJu commented Feb 7, 2018

I confirm it fixes #21444

@sheetalkamat
Copy link
Member Author

@mhegazy Ready for merge pending your approval

@mhegazy
Copy link
Contributor

mhegazy commented Feb 7, 2018

Please port to release-2.7 as well.

@sheetalkamat sheetalkamat merged commit 3b1a3f8 into master Feb 7, 2018
@sheetalkamat sheetalkamat deleted the fileChangeThroughDeleteAndCreate branch February 7, 2018 18:24
@avbentem
Copy link

Tested on a (slow) Windows 7 with quite a few corporate virus scanners running: 2.8.0-dev.20180215 fixes the issues I had with WebStorm's "safe write" option enabled. (Which creates temporary files, deletes the original and then renames the temporary file.)

Thanks for this fix!

@microsoft microsoft locked and limited conversation to collaborators Jul 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants