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

Directories dont check modified time when sending "change" event #57938

Merged
merged 2 commits into from Mar 25, 2024

Conversation

sheetalkamat
Copy link
Member

@sheetalkamat sheetalkamat commented Mar 25, 2024

When checking whether to send event invoked with fs.watch we shouldnt do that for directory. Directory can send change event for multiple reasons and its not right to filter that out.

a7a308d Adds the failing test. It modifies out vfs sending incorrect timestamps, incorrect events etc.
88c5588 actual fix

Fixes #57792

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Mar 25, 2024
@sheetalkamat sheetalkamat changed the title Use fs events on parent directory Directories dont check modified time when sending "change" event Mar 25, 2024
Base automatically changed from watchTestsTimeStamp to main March 25, 2024 21:11
@sheetalkamat sheetalkamat reopened this Mar 25, 2024
Directories generate change event in multiple scenarios and should not filter events based on modified time
Fixes #57792
Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

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

Does this need a backport to 5.4?

@sheetalkamat sheetalkamat merged commit d4b8ff6 into main Mar 25, 2024
25 checks passed
@sheetalkamat sheetalkamat deleted the useFsEventsOnParentDirectory branch March 25, 2024 23:19
@sheetalkamat
Copy link
Member Author

@DanielRosenwasser @RyanCavanaugh do we want this for 5.4 ? If we need to port this, there will be test breaks unless we take #57936

@jakebailey
Copy link
Member

The cherry pick task will fix up the baselines, so that all would be handled (but we can still manually fix it)

@NMinhNguyen
Copy link
Contributor

Just to check - is this going to be backported to 5.4 or only 5.5?

@jakebailey
Copy link
Member

I think it's worth considering.

@typescript-bot cherry-pick this to release-5.4

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 27, 2024

Starting jobs; this comment will be updated as builds start and complete.

Command Status Results
cherry-pick this to release-5.4 ✅ Started ✅ Results

@typescript-bot
Copy link
Collaborator

Hey, @jakebailey! I've created #57958 for you.

DanielRosenwasser pushed a commit that referenced this pull request Mar 28, 2024
…e-5.4 (#57958)

Co-authored-by: Sheetal Nandi <shkamat@microsoft.com>
Co-authored-by: Jake Bailey <5341706+jakebailey@users.noreply.github.com>
@DanielRosenwasser DanielRosenwasser added this to the TypeScript 5.4.32 milestone Mar 28, 2024
@NMinhNguyen
Copy link
Contributor

Apologies for the direct mention @jakebailey, but is a new 5.4 version still planned to go out at some point?

@jakebailey
Copy link
Member

This was in 5.4.4 which came out last week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tsc --watch via directory-level fsevents not working on macOS 14.4
5 participants