refactor: Handle project definition changes#1478
Draft
RandomByte wants to merge 24 commits into
Draft
Conversation
Reorder the resets in ProjectBuildCache#discardIncrementalState so the load-bearing ones come first, each annotated with why it is required, and group the two that only exist for completeness (#sourceIndex, #cachedResultSignature) at the end under a single comment: both are overwritten before read on the next build (#sourceIndex by #initSourceIndex, #cachedResultSignature by #findResultCache). Drop the #changedProjectSourcePaths and #writtenResultResourcePaths resets. Setting #combinedIndexState to RESTORING_PROJECT_INDICES re-arms initSourceIndex, which the next build runs before validateCache and which re-clears (#changedProjectSourcePaths) and reassigns (#writtenResultResourcePaths) both fields from a fresh disk read. No behavior change: every caller follows discardIncrementalState with a full build that routes through initSourceIndex.
Introduce a ServeSupervisor that owns a stable http.Server and re-creates the
whole serving stack (graph + Express app + BuildServer) behind a request
trampoline. serve() delegates to it and returns a reinitialize() handle
alongside the existing {h2, port, close}.
The BuildServer freezes a project's definition (ui5.yaml, package.json,
ui5-workspace.yaml) at startup, so a branch switch rebuilds new sources against
the old config. A reader-level fix cannot help: MiddlewareManager reads
server.customMiddleware and resolves extensions once, baking them into the
Express chain. Re-creating the stack above @ui5/server refreshes both the build
model and the server config.
Re-init is build-new-then-swap: the new graph and app are built before the old
BuildServer is destroyed, so an invalid ui5.yaml leaves the last-good app
serving. The port stays bound and live-reload clients stay connected: requests
route through the trampoline to the current app, and live-reload subscribes to a
relay whose upstream retargets on swap. The config-aware build signature keeps
the refcounted cache warm across the swap.
- Extract buildServeApp (serveApp.js) and the HTTP helpers (serveHttp.js) so the
supervisor and the single-shot wrapper share one construction path.
- Thread a graphFactory closure from the CLI so the graph re-resolves with
identical parameters; the returned reinitialize() is captured for the future
definition watcher.
The definition watcher that will call reinitialize() is a follow-up.
…en() The changePortIfInUse if/else in listen() built the same error object twice: identical code, errno, address and port assignments differing only in the message string, and computed portMax through a mutable let. Compute the message conditionally and construct the error once; derive portMax with a ternary. Carried over verbatim from the former server.js _listen; the extraction into serveHttp.js is where it collapses.
Remove three pieces of surface that no caller reaches: - The h2 getter and its #h2 backing field. serve() returns the locally destructured h2 and #init passes the local h2 to announceListening, so the getter was never read. - getServeError(). The serve-error signal reaches the middleware through serveApp.js, which closes over the BuildServer directly; the supervisor method had no production caller. - The static #peek / __internals__ introspection hole and its NODE_ENV branch. The one test that used it now asserts observable behavior: the trampoline still routes to the last-good app after a failed re-init, matching the neighboring trampoline test. Move generateWebSocketToken back inline into serve(). The supervisor never called it; server.js generated the token via the static and threaded it back in through config.webSocketToken. Keeping the generate-once-thread-through-config story in server.js, which already owns config assembly, drops the cross-file bounce and an unused static.
Watch the project-definition files (ui5.yaml, package.json, the workspace config, and in static-graph mode the dependency-definition file) and emit a settled, coalesced "definitionChanged" when one changes. Exported via a new "./build/helpers/DefinitionWatcher" subpath, mirroring "./build/cache/Cache". The watch model is include-based: @parcel/watcher subscribes to each distinct project root and the callback drops every event whose path is not in the resolved definition-file set. The node_modules/.git ignore globs only reduce OS-level watch load; correctness comes from the include set. The settle window is trailing-only (a re-init needs no leading edge) and sized at 550 ms to sit above @parcel/watcher's 500 ms MAX_WAIT_TIME, so a git checkout that writes several files collapses to a single emit. Error recovery mirrors BuildServer's #recoverWatcher (a re-entrancy guard plus a budget of 5 attempts / 60 s), but the budget is the watcher's own, independent of the source WatchHandler. Separate from the source WatchHandler: source events drive incremental rebuilds inside the BuildServer, definition events drive a full re-init above it.
Wire a DefinitionWatcher into the ServeSupervisor lifecycle so an on-disk change to a definition file re-creates the serving stack. Until now reinitialize() was only callable programmatically, so a git checkout or a hand-edit of ui5.yaml did not refresh a running server. The watcher is owned by the supervisor, not the BuildServer: it outlives each swapped-out stack and is re-targeted to the new graph after every swap, since the project set or their roots may have changed. It is armed at the tail of #init (after the port is bound) and only when a graphFactory is present, since reinitialize() is otherwise a no-op. A watcher-create failure during a swap is logged and does not crash the swap; the old watcher keeps driving re-inits. destroy() stops the watcher before the socket closes, so a late event cannot start a re-init mid-teardown. rootConfigPath, workspaceConfigPath, dependencyDefinitionPath and cwd are threaded through serve() into the supervisor config so the watcher can locate a custom root config, the workspace file, and the static dependency-definition file. Existing callers omit them and degrade to watching ui5.yaml/package.json at project roots.
Pass the argv values that already feed buildGraph (argv.config, argv.workspaceConfig, argv.dependencyDefinition) into serverConfig so they also reach the server's definition watcher. Workspace resolution is active unless static-graph mode is used or --workspace is disabled; pass null then (undefined to skip) so the watcher applies its default ui5-workspace.yaml even when --workspace-config was not given. Static-graph mode omits the workspace, matching how buildGraph resolves the graph.
Split the router-agnostic middleware-assembly core out of buildServeApp into an
exported buildServeRouter(graph, config, error) returning {router, buildServer,
liveReloadOptions}. It mounts every UI5 middleware onto an express.Router() (which
both an Express app and a Connect app accept via use()), stopping short
of creating the express() app and the terminal error handler.
buildServeApp now wraps buildServeRouter: it creates the express() app, mounts
the router, and appends createErrorHandler. Its {app, buildServer,
liveReloadOptions} return contract is unchanged, so ServeSupervisor is untouched.
This isolates the two HTTP-server-owned concerns (the terminal error handler
and, via ServeSupervisor, the live-reload WebSocket server) from the middleware
core, so an embedding API can reuse the core without them.
Add a public serveMiddleware(graph, options, error) returning {middleware,
buildServer, close} so consumers running their own express/connect server (e.g.
karma-ui5) can mount UI5's middleware with a single app.use(middleware), rather
than importing @ui5/server/internal/MiddlewareManager and hand-assembling
readers.
serveMiddleware delegates to buildServeRouter and returns its router. It accepts
only the middleware-relevant options (sendSAPTargetCSP, simpleIndex,
serveCSPReports, cache, ui5DataDir, includedTasks, excludedTasks); port, h2/SSL,
remote-connection, and definition-watcher options belong to the CLI-owned
listener. Live-reload stays in place but dormant with no token. No terminal
error handler and no live-reload WebSocket server are attached: the caller owns
the HTTP server and its error handling.
close() releases the BuildServer's source watcher and build-cache handle and is
idempotent. A graph can be served only once, so serveMiddleware and serve must
not run against the same graph.
Exported as a named export from lib/server.js alongside serve().
The InteractiveConsole writer's model is single-root-project, but the root can be resolved more than once in a process: ServeSupervisor.reinitialize() re-resolves the graph on a project-definition change (a git checkout, a hand-edit of ui5.yaml) and re-emits ui5.project-resolved for the same root. projectGraphBuilder emits the event on every build. Previously the writer latched #seenProjectResolved on the first event and threw "Received duplicate ui5.project-resolved event" on the second. Because process.emit runs listeners synchronously, that throw propagated out of the graph factory into ServeSupervisor.#swap and aborted every re-init whenever the interactive writer was active (the default for `ui5 serve` in a TTY): the feature was inert. Treat a repeat as an in-place update: the framework name/version and project type may have changed across the switch, so setProject refreshes the region. This mirrors the non-interactive Console writer, which already logs a fresh info line per event.
A git checkout that moves or removes a watched source directory while `ui5 serve` is running escalated the server to a terminal ERROR. Two defects in WatchHandler, both independent of the definition-watcher re-init that is the primary branch-switch recovery path: 1. #handleWatchEvents called getVirtualPath for every event. On a diverged tree the path no longer maps into the project the watcher was armed with, so getVirtualPath threw "Unable to convert source path ... to virtual path", and the per-event catch turned that into a fatal watcher error. 2. Recovery re-subscribed over the same graph's paths; parcelWatcher.subscribe on a removed .../src directory rejected with "No such file or directory", which propagated to #setState(ERROR). Drop an event whose path no longer maps to a virtual path (verbose log), and skip a subscribe path that no longer exists on disk (warn log). The skip reads current disk state via fs access rather than matching parcel's locale-dependent, code-less message; a subscribe failure on a path that still exists is a genuine fault and still propagates into BuildServer's loop-protected recovery/escalation budget.
The interactive console's Project region shows the root project version read from package.json. While `ui5 serve` runs and a definition change is known to be coming (a branch switch, a ui5.yaml edit), the displayed version is stale until the graph re-resolves: the settle window plus the resolve time, seconds on a large project. Add two process events that bracket a re-resolve. `ui5.project-resolving` blanks the version slot(s) to a dim `resolving…` placeholder while the project name and type keep showing, so the region does not collapse. This is distinct from the startup `showPlaceholders` reservation (driven by `ui5.tool-mode`), which placeholders the whole region before any data arrives. The placeholder is released one of two ways. A completed resolve arrives as `ui5.project-resolved` and repopulates the slot with the new version (via `setProject`). A re-resolve abandoned without completing arrives as `ui5.project-resolve-failed`, which releases the placeholder back to the last-known version rather than leaving it on `resolving…` indefinitely. The framework version goes stale alongside the project version on a branch switch, so it is placeholdered too, keeping the framework name.
DefinitionWatcher coalesces a definition-file burst (a git checkout writes ui5.yaml + package.json + sources across batches) into a single trailing definitionChanged, a settle window after the last event. That is the right moment to re-resolve the graph, but it leaves the window between the first change and the settled emit with no signal that a re-init is coming. Emit a definitionChanging on the leading edge (the first watched event of a burst, detected as no settle timer being armed yet) before arming the timer. The trailing definitionChanged is unchanged. An owner uses this to react to the pending change (e.g. show that the resolved version is about to change) ahead of the re-resolve, without a second event per batch: further events within the window reset the settle timer but do not re-fire definitionChanging.
… swap
The interactive console's Project region shows the root project version. On a
branch switch the definition watcher fires, the graph re-resolves, and the new
version paints, but the window between the change and the resolve shows the stale
version.
Wire the watcher's leading-edge definitionChanging to
process.emit("ui5.project-resolving"), so the Project region blanks the version
slot to a "resolving…" placeholder from the moment a change is known. The swap's
own graph resolution emits ui5.project-resolved and repopulates the slot with the
new version naturally, so the placeholder needs no early re-resolve to clear it.
The subscription is attached in #startDefinitionWatcher alongside the
definitionChanged handler, so it survives watcher re-targeting after each swap.
A resolve that fails never emits ui5.project-resolved, so the #swap failure path
emits ui5.project-resolve-failed to release the placeholder back to the
last-known version the still-serving stack resolved.
…er request The post-abort/transient restart window (ABORTED_BUILD_RESTART_SETTLE_MS) exists to collapse a change burst delivered as multiple @parcel/watcher batches (a `git checkout`, a save-all) into one rebuild against the settled tree. All three timing paths share one timer (#processBuildRequestsTimeout), and #triggerRequestQueue unconditionally clears and re-arms it, so whoever calls last wins. While a restart is deferred, a browser's live-reload keeps requesting resources. Each request for a non-fresh project routes through BUILD_REQUEST_DEBOUNCE_MS. That re-arm supersedes the 550 ms restart window: if no further source change lands within the debounce (a gap between watcher batches), the timer fires and a build starts into the still-arriving burst, only to be aborted by the next batch. In the openui5 testsuite switching between `master` and `rel-1.120`, a single checkout produced seven builds, six of them aborted, instead of one. While #pendingDeferredRestart is set, #enqueueBuild now leaves the armed timer untouched: the project is still queued on its status and resolves when the deferred rebuild — which processes the whole pending set — runs. Leaving the timer alone (rather than re-arming it at the settle delay) also avoids the opposite failure, where a steady stream of live-reload requests would reset the window and defer the rebuild indefinitely. The window is reset only by further source changes, as before. A reader request still supersedes the shorter first-build window.
…th watchers WatchHandler.destroy() and DefinitionWatcher (both destroy() and the recovery teardown) each drained their @parcel/watcher subscriptions with the same Promise.allSettled(unsubscribe) + collect-rejections idiom. Lift it into drainSubscriptions() in helpers/watchSubscriptions.js. The helper returns the rejection reasons rather than shaping the error, so each caller keeps its own contract: destroy() aggregates the failures into an AggregateError it emits, while the DefinitionWatcher recovery path discards them (the handles are being renewed regardless).
SOURCES_CHANGED_SETTLE_MS, ABORTED_BUILD_RESTART_SETTLE_MS (BuildServer) and DEFINITION_CHANGED_SETTLE_MS (DefinitionWatcher) all held 550 ms for the same reason: a burst-coalescing settle window must sit above @parcel/watcher's MAX_WAIT_TIME (500 ms) so each batch resets the window rather than terminating it. The value and the paragraph explaining it were duplicated three times, and all three necessarily move together if the watcher's cap changes. Define WATCHER_BURST_SETTLE_MS once in helpers/watchSettle.js, carrying the @parcel/watcher-coalescing rationale. The three call sites alias it and keep only a one-line note on what each window coalesces (leading+trailing live-reload emit, post-abort rebuild, trailing-only re-init). The BuildServer aliases preserve the existing names, so the __internals__ test contract is unchanged.
BuildServer's source-watcher recovery and DefinitionWatcher each kept their own sliding-window loop protection: a `#…RecoveryTimestamps` array, the prune-to- window + `>= MAX` check, and a private copy of WATCHER_RECOVERY_MAX_ATTEMPTS / WATCHER_RECOVERY_WINDOW_MS. The bookkeeping and the two constants were identical. Lift the window accounting into RecoveryBudget (helpers/RecoveryBudget.js): withinBudget() prunes and reports whether another attempt fits, recordRecovery() marks a completed one. Each site holds its own budget instance, so a fault in one watcher does not consume the other's allowance. The divergent parts stay with each owner: the re-entrancy guard (in BuildServer `#recoveringWatcher` also gates the request queue, so it is not recovery-local), the recovery body (BuildServer quiesces the build and forces a full re-scan; DefinitionWatcher re-subscribes its directory set), and the escalation on exhaustion (a state transition to ERROR vs. a terminal "error" emit).
buildServeRouter() starts the BuildServer via graph.serve() before assembling the middleware. If MiddlewareManager.applyMiddleware() throws afterwards, the rejection propagated without a buildServer or close() handle, leaving the source watcher and build-cache handle alive. Wrap the middleware assembly in try/catch, destroy the BuildServer, then rethrow the original error. Covers both the serveMiddleware() embedding API and the CLI-owned buildServeApp() path, since both route through buildServeRouter().
serveMiddleware() is the public embedding API: it hides the reader/resource/ middleware setup and returns a mountable middleware plus close(). Add an @example to its JSDoc showing the full lifecycle (import, app.use(middleware), close() on teardown) so third-party consumers see the supported contract in the generated API reference, without importing @ui5/server/internal/MiddlewareManager or recreating the old workaround.
The serve-prefixed module names carried a manual namespace inside a package already called @ui5/server, and the prefix read as a verb while the modules are factories and helpers. Move the internals into a serve/ directory so the prefix is gone and the names describe their contents, and reserve the top level for the two public entries (server.js, serveMiddleware.js): serveHttp.js -> serve/httpListener.js serveApp.js -> serve/stack.js ServeSupervisor.js -> serve/Supervisor.js Drop the now-redundant Serve from the moved symbols, which the namespace already scopes: the stack exports buildServeRouter -> buildRouter and buildServeApp -> buildApp, and the class ServeSupervisor -> Supervisor (logger tag server:Supervisor). No public API change; the package.json exports and the serve()/serveMiddleware() surface are untouched.
Both surfaces become committed contracts once merged, so align their names before they ship rather than accrete inconsistency as they grow. Module: rename DefinitionWatcher -> ProjectDefinitionWatcher and move it out of build/helpers/ up to graph/. It watches the project-definition files (ui5.yaml, package.json, workspace config) and drives graph re-resolution, a project-graph concern rather than a build-pipeline helper, and it is the only public export in that directory. The export path becomes @ui5/project/graph/ProjectDefinitionWatcher. Signal: rename the branch-new process-bus signal ui5.project-resolving -> ui5.project-resolve-started, so the resolve lifecycle reads as a uniform verb-state family (-resolve-started / -resolve-failed). ui5.project-framework-resolved is left as-is: it is a one-shot notification, not a start/success/fail lifecycle. Also correct the stale ServeSupervisor references in docs and comments to the shipped class name Supervisor.
ui5.project-resolved shipped on main as the single terminal resolve signal. The branch completes the lifecycle around it (a leading ui5.project-resolve-started and a ui5.project-resolve-failed), which exposed a grammatical mismatch: gerund vs past-participle vs verb-state. Rename the terminal signal to ui5.project-resolve-succeeded so the family reads as a uniform verb-state set. This is the expensive half of the alignment (renaming a name that already shipped): the emitter in projectGraphBuilder.js plus both the Console and InteractiveConsole writers and their tests. Split from the branch-introduced renames so the pre-existing contract change is reviewable on its own.
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
1 participant
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.
Problem
ui5 servecould not safely react to project-definition changes while running. Changes to files likeui5.yaml,package.json, workspace config, or a static dependency definition required a fresh project graph, but the server only had a source watcher and an already-built serving stack. Branch switches or config edits could leave the running server pointed at stale graph state, stale source paths, or transient watcher/build errors.Solution
This adds a definition-change path next to the existing source-change path.
ProjectDefinitionWatcherfor project-definition files.Supervisorto keep the HTTP server and port stable while rebuilding the graph, Express app, andBuildServerbehind it.serveMiddlewarefor mounting UI5 middleware into an external server.ProjectDefinitionWatcher and Supervisor
ProjectDefinitionWatcherwatches the files that define the project graph: projectpackage.jsonfiles,ui5.yamlor custom--config, workspace config, and static dependency-definition files. It emits an earlydefinitionChangingsignal for UI feedback and a settleddefinitionChangedsignal that drives server reinitialization after change bursts such as branch switches.The watcher is owned by
Supervisorrather than theBuildServer, so it survives serving-stack swaps. After a successful reinitialization it is retargeted to the new graph, while the source watcher stays focused on incremental rebuilds for the currently active graph.Supervisorholds the socket and the relay permanently. Everything else -- the serving app, theBuildServer, and theProjectDefinitionWatcher-- is rebuilt on a definition change and swapped behind the request forwarder without dropping the port or connected browsers.Middleware API
The server now separates middleware assembly from HTTP listener ownership. This is what makes the project graph swap possible: the HTTP server stays bound while a new graph, middleware stack, and
BuildServerare created and swapped in behind it.serve()still starts and owns the UI5 dev server, but internally uses reusable pieces for building the middleware stack, binding HTTP, and supervising graph swaps.This also adds the new
serveMiddleware()API for third-party consumers. It builds the UI5 reader/resource setup and applies the same standard and custom middleware chain thatui5 serveuses, but returns it as a connect/Express-compatible middleware instead of starting an HTTP server.External tools can now do
app.use(result.middleware)on their own Express/connect server and keep ownership of the port, protocol, routing around UI5, and error handling. The returnedclose()hook releases the underlyingBuildServerwatcher and cache resources when the external server shuts down.The caller owns the port, the HTTP server, error handling, and teardown order.
serveMiddlewarenever binds a socket, never attaches a live-reload WebSocket server, and never installs a terminal error handler -- those stay with the caller's server.