feat(builders): make watching npm-linked shared deps opt-in - #131
Merged
Conversation
Core 4.5.0 gates linkedSharedDirs behind a new watchLinkedDeps federation option that defaults to false. Under pnpm's default linker every dependency is a symlink, so the npm-link watch added in 4.3.2 treated the whole graph as linked: startup stalled on checksum walks and an idle ng serve kept polling. Neither builder built that field into its options object, so linkedSharedDirs returned [] and nobody could turn the watch back on. Correctness never depended on the watch -- linked content signals still run unconditionally, so a rebuilt linked library re-bundles on the next build regardless. The option only buys live reload. Also supply watchpack as the watch implementation. Core's built-in poll is dependency-free but re-walks the tree every 300ms; watchpack is event-driven and survives having a watched directory swapped out, which is what `ng build <lib>` does to dist and what a raw fs.watch silently dies on. Refs #130
Detection now keys off a symlink whose real path resolves outside every node_modules tree, so a package manager's own symlinks no longer count as linked, and watching is opt-in via watchLinkedDeps. Also spell out that the option does not gate correctness, and that preserveSymlinks is not the switch for it -- it changes esbuild's resolution and invites duplicate singletons. Refs #130
The port's JSDoc claimed watchpack replaces a per-interval tree walk, but
core always passes `{ poll: true }` for linked dirs and watchpack then does
the same readdir+lstat sweep per directory. Its real advantage is surviving
the `dist` swap `ng build <lib>` does, which is what the comment now says.
`aggregateTimeout: 0` only gated the `aggregated` event, which nothing
subscribes to; `change`/`remove` fire synchronously either way. Dropped.
Documented why `poll` must be forwarded (without it the swap is reported
once and the watch dies on the deleted inode) and why `followSymlinks`
stays false while Angular threads `preserveSymlinks` into it.
The README claimed `preserveSymlinks` is unrelated to this. Angular in fact
uses it as exactly this switch, skipping its `**/node_modules/**` ignore so
`npm link` keeps working; it overloads one flag because its resolver picks
the path esbuild sees. Keep the guidance, fix the reason - and note that
turning it on would shrink the watch set, since paths with a node_modules
segment are skipped.
Three spots claimed more than the code does. The depth-1 `ignored` predicate does not stop watchpack descending — it only collapses a deeper change onto its depth-1 ancestor, which core's file watch then drops as untracked. fs.watch reports nothing there; this reports the ancestor. The test asserted only that no nested path appeared, so it passed while the ancestor event fired unremarked; it now pins that behaviour from two levels down, where a parent mtime cannot explain it. "Correctness is not affected" held for a cold build but not inside ng serve: rebuildAffectedExternals returns before bundleShared when no modified file falls under the package dir, so a JavaScript-only change can stay stale until a restart. And off is not "no reload" either — a library's .d.ts is a TypeScript input resolved outside node_modules, so an ng-packagr rebuild is already picked up without the option.
watchLinkedDeps defaults to false, so anyone who npm-links a shared library loses the reload that shipped in 22.0.6 with nothing said. That reads as a broken build rather than a default, since a linked library and a registry one look identical from the host. A watching build now names what it found once at startup. linkedSharedDirs returns [] when the option is off, so ask it again with the option forced on: the walk behind it is findDepPackageJson + realpath + lstat per shared key, not the recursive content walk that made #130 slow. Silent when nothing is linked, when the option is already on, and for a package manager's own symlinks -- a pnpm tree stays quiet.
The hint lived here and was called from both builders, which meant every other adapter had to reimplement it. Core now emits it from buildForFederation, so the builders drop their call and this workspace keeps no copy of the logic. The message loses "on this target" with the move; core has no notion of targets.
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
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.
Plumbs core's new
watchLinkedDepsoption through both builders, and replaces core's built-in poll with watchpack.Refs #130 (already closed by the core-side fix — this is the adapter half).
Why
The
npm linksupport added in 4.3.2 decided a shared dependency was a live dev checkout by asking whethernode_modules/<pkg>was a symlink. Under pnpm's default linker every dependency is one, so the whole graph was treated as linked andng servenever finished starting.Core 4.5.0 fixes the detection (a symlink counts only when its real path sits outside every
node_modulestree) and, separately, makes the watch opt-in via a newwatchLinkedDepsfederation option that defaults tofalse. Neither builder built that field into its options object, solinkedSharedDirsreturned[]and nobody could turn the watch back on.What's in here
watchLinkedDepson both builders —schema.json+schema.d.tsfor:buildand:remote, passed intonormalizeFederationOptionsbesidewatch. Defaults tofalse.WatchPort(src/utils/watchpack-watch.ts), wired intocreateNfWatcherinbuild/builder.tsandremote/change-watcher.ts. Core's poll is dependency-free but re-walks the tree every 300 ms; watchpack is event-driven, and unlike a rawfs.watchit survives having a watched directory swapped out — which is exactly whatng build <lib>does todist, and why core polls rather than usingfs.watchitself. It also picks up a directory that only appears after the watch starts, whichfs.watchcannot — the case where you startng servebefore the library's first build.npm linked shared libraries" section now covers the new detection rule, the opt-in, and thatpreserveSymlinksis not the switch for this.What the option actually gates
Worth being precise, because the obvious reading is too strong in both directions.
watchLinkedDeps: falsedoes not mean "edits to the linked library are ignored". A library's.d.tsis a TypeScript program input, and the adapter resolves it to its real path, which by definition lies outsidenode_modules— so it is already in the watch set throughfederationSourceFiles, flag or no flag. An ng-packagr rebuild rewritesdist/*.d.tsalongside the.js, that lands inmodifiedFiles, andrebuildAffectedExternalsmatches it against the package dir and re-bundles. In practice most linked-library workflows keep live reload with the flag off. What the flag adds is coverage of changes that touch no watched TypeScript input — a.js-only edit, a rebuild that leaves the emitted types byte-identical.Correctness is not gated, but "the next build" means a cold one.
linkedContentSignalsruns unconditionally insidebundleShared, so a freshng buildalways re-bundles a changed linked library. Inside a runningng serveit is weaker:rebuildAffectedExternalsreturns early when no modified file falls under the package dir, beforebundleSharedis ever reached. So with the flag off, a.js-only change to a linked library can stay stale in the dev server across arbitrarily many unrelated rebuilds, until the server restarts or a cold build runs. That is core's shape, not something this PR introduces, and it is exactly the hole the flag closes — but it is not "never served from a stale cache".The
recursive: falseandpolldecisionsopts.recursive: falseis honoured through watchpack'signoredpredicate, but only in what it reports, not in how far it descends. Watchpack watches the subtree either way; the predicate collapses a deep change onto its depth-1 ancestor. Probing a depth-3 modification that left every parent directory's mtime untouched:This is safe as things stand, because core's only non-recursive watch is the file-directory watch, which filters every event through
trackedFiles— a collapsed ancestor path is simply dropped. It is not, however, anfs.watch-equivalent cap, and a future core change that used a non-recursive directory watch would see the collapsed path. Flagging it rather than burying it.opts.pollis honoured rather than ignored. The CPU win comes from the source-file watches, which arrive with no poll hint; linked dirs are small, and core'ssupersede()assumes a polled directory watch really is inode-replacement-safe, so dropping the hint there would be the one place a missed rename-replace never gets re-covered.One consequence of the seam worth knowing:
watchpackWatchnever throws, including on a path that does not exist. Core'stry/catcharoundwatch()and itswatchFailed()warning are therefore unreachable on this path — on inotify exhaustion watchpack prints its ownWatchpack Error (watcher): …and treats the directory as removed, so a watch dies with a different message than before. No unhandled-error-event risk:DirectoryWatcher.onWatcherErrorabsorbs it.Behaviour change
This turns off live-reload for the subset of linked-library changes that touch no watched TypeScript input — most visibly a
.js-only edit — which shipped on in22.0.6. Nothing breaks and no types change, but anyone relying on it will find it stopped until they setwatchLinkedDeps: true. Worth an explicit release note.Verification
6 new tests in
src/utils/watchpack-watch.spec.tscover the adapter, including the directory-swap case. Full suite: 188 passing.tsc --noEmit, eslint and knip clean.Also run end to end against
angular-examples/angular/simpleon pnpm's default isolated linker (27 of 27 dependencies symlinked — the exact shape that regressed in #130), with the localdist/installed through afile:override so core4.5.0-next.1and watchpack2.5.2resolve for real. The linked dependency is a package carrying an ordinary semver range inpackage.jsonand a hand-made symlink innode_moduleswhose real path resolves outside everynode_modulestree; its build script writes a staging directory andmvs it overdist/, reproducing ng-packagr's inode replacement rather than rewriting in place.ng serveon pnpm-default, flag offng serve, flag on.js-only edit in the linkeddist.d.ts+.js).d.tsTypeScript input.js-only edit, then an unrelated source editng build.js-only edit in the linkeddistdistdirectory swapswatchLinkedDepson:buildand on:serve;ng build mfe1Not covered: the
:remotebuilder has no end-to-end exercise here (this workspace has no:remotetargets), so it rests on the sharedwatchpackWatchunit tests. The watchpack-versus-core-poll CPU comparison was not A/B'd againstmaineither — the case for the swap is the directory-swap survival above, which is measured.