fix(ext/node): emit 'error' event for fs.watch open failures#34398
Merged
Conversation
`fs.watch` would throw a raw `Deno.errors.NotFound` synchronously when the underlying inotify/FSEvents watch couldn't be added (commonly when an editor's atomic-save renames its temp file away before the watch is installed). EventEmitter-style consumers like chokidar/vite expect a Node-style ENOENT delivered via the watcher's 'error' event, so the sync throw surfaced as an uncatchable rejection inside vite's async file handler. Wrap the `Deno.watchFs` call, convert NotFound to a Node-style ENOENT `uvException` (the notify crate's error message lacks the `(os error N)` suffix that `denoErrorToNodeError` parses), and emit it on the returned FSWatcher via `process.nextTick` so listeners attached after `watch()` returns can handle it. Fixes #34396
fibibot
requested changes
May 26, 2026
fibibot
left a comment
fibibot
left a comment
Contributor
There was a problem hiding this comment.
This needs one cleanup before merge: tools/lint_plugins/no_deno_api_in_polyfills.ts says new Deno.* usage in Node polyfills is not allowed, but this PR raises ext/node/polyfills/fs.ts from 51 to 52 for the new Deno.errors.NotFound check. Please keep the baseline from increasing, for example by moving the watch-open error mapping behind an internal helper/op or otherwise avoiding a new Deno namespace access in fs.ts.
Move the NotFound -> ENOENT mapping out of ext/node/polyfills/fs.ts and into denoErrorToNodeError in internal/errors.ts, so fs.ts doesn't need a new Deno.errors.NotFound access. Keeps the no-deno-api baseline at 51 for fs.ts. Also clears the test's 5s timeout so the leak detector is happy.
…eError" This reverts commit 6705435.
fibibot
requested changes
May 27, 2026
fibibot
left a comment
fibibot
left a comment
Contributor
There was a problem hiding this comment.
"ext/node/polyfills/fs.ts": 52still raises theno_deno_api_in_polyfillsbaseline for the newDeno.errors.NotFoundcheck. This file’s rule says newDeno.*usage in Node polyfills is not allowed; keep the baseline from increasing, for example by moving the NotFound mapping behind an internal helper/op or otherwise avoiding a newDenonamespace access infs.ts.
The new unit test covers the async ENOENT event shape, but the baseline increase is still blocking.
On Windows, Deno.watchFs() may not throw synchronously for a missing path; the error surfaces asynchronously through the async iterator. The iterator error callback was emitting the raw Deno error without converting it to a Node-style error, so err.code was undefined. Refactor the NotFound-to-ENOENT mapping into a shared helper used by both the synchronous catch block and the iterator error callback.
fibibot
requested changes
May 27, 2026
fibibot
left a comment
fibibot
left a comment
Contributor
There was a problem hiding this comment.
"ext/node/polyfills/fs.ts": 52still raises theno_deno_api_in_polyfillsbaseline for the newDeno.errors.NotFound.prototypeaccess infs.ts. The rule is specifically there to prevent newDeno.*usage in Node polyfills; keep the count at 51 by moving this NotFound classification behind an internal helper/op or another existing abstraction that does not add a polyfill-sideDenonamespace access.
The async ENOENT event behavior and regression test are the right shape, but the baseline increase remains blocking.
fibibot
requested changes
May 27, 2026
fibibot
left a comment
fibibot
left a comment
Contributor
There was a problem hiding this comment.
"ext/node/polyfills/fs.ts": 52still raises theno_deno_api_in_polyfillsbaseline from 51, and the new head adds another polyfill-side access withDeno.lstatSync(watchPath);. The rule explicitly says adding newDeno.*usage is not allowed; keep this path behind an internal helper/op or another existing abstraction so the baseline does not increase.
The fs.watch ENOENT event behavior is still covered by the new unit test, but the baseline increase remains blocking.
fibibot
previously requested changes
May 27, 2026
fibibot
left a comment
fibibot
left a comment
Contributor
There was a problem hiding this comment.
"ext/node/polyfills/fs.ts": 53still raises theno_deno_api_in_polyfillsbaseline, and the new head adds two polyfill-sideDeno.*uses:Deno.errors.NotFound.prototypeandDeno.lstatSync(watchPath). The lint rule says this count must only go down; please keep the watch-open ENOENT mapping behind an internal helper/op or another existing abstraction sofs.tsdoes not add newDenonamespace accesses.
The async fs.watch ENOENT behavior is still covered by the unit test, but the baseline increase remains blocking.
littledivy
pushed a commit
to crowlKats/deno
that referenced
this pull request
Jun 10, 2026
…d#34398) `fs.watch` was throwing `Deno.errors.NotFound` synchronously when the underlying inotify/FSEvents watch couldn't be added — most commonly when an editor's atomic-save renamed its temp file away before the watch was installed. EventEmitter-style consumers like chokidar (and vite, which bundles it) expect a Node-style ENOENT delivered via the watcher's `'error'` event, so the synchronous throw surfaced as an uncatchable rejection inside vite's async file handler. The fix wraps the `Deno.watchFs` call, converts `NotFound` to a Node-style ENOENT `uvException` (the notify crate's error message lacks the `(os error N)` suffix that `denoErrorToNodeError` parses, so we detect it by class), and emits it on the returned `FSWatcher` via `process.nextTick` so listeners attached after `watch()` returns can handle it. Fixes denoland#34396
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fs.watchwas throwingDeno.errors.NotFoundsynchronously when theunderlying inotify/FSEvents watch couldn't be added — most commonly when
an editor's atomic-save renamed its temp file away before the watch was
installed. EventEmitter-style consumers like chokidar (and vite, which
bundles it) expect a Node-style ENOENT delivered via the watcher's
'error'event, so the synchronous throw surfaced as an uncatchablerejection inside vite's async file handler.
The fix wraps the
Deno.watchFscall, convertsNotFoundto aNode-style ENOENT
uvException(the notify crate's error message lacksthe
(os error N)suffix thatdenoErrorToNodeErrorparses, so wedetect it by class), and emits it on the returned
FSWatcherviaprocess.nextTickso listeners attached afterwatch()returns canhandle it.
Fixes #34396