Keep modulepreload hints within the servable set#161
Conversation
The SSR preload emitter walks transitiveDeps, the source-serving authorization gate walks reachableFromEntries. The two disagreed about the .server.* boundary, so the framework emitted modulepreload hints for URLs it then 404s. Two root causes, both surfaced by auditing the four dogfood apps for broken preloads: 1. transitiveDeps walked THROUGH .server.* files into their server-only deps (slugify.ts, the two types.ts on the blog). The gate stops at that boundary; the preload walk did not. Add the same SERVER_FILE_RE stop so a preload set is always a subset of the servable set. The .server.* file itself stays preloadable (its stub URL is fetched); a file reachable through both a server file and a real client path is still reached via the client path. Closes #158. 2. The import scanner matched import/export statements that appear as TEXT inside a template literal (example code in a <pre> inside an html`` template, as the docs getting-started page does), producing a phantom edge and a preload for a file that does not exist (/app/docs/components/counter.ts on docs). Reuse js-scan's redactStringsAndTemplates as a position-preserving mask and skip any match whose keyword was blanked (sits inside a literal). The specifier is still read from raw source, so real top-level imports and multi-line export-from re-exports are unaffected. Closes #159. Audit result across the four apps: blog had the 3 server-only 404s, docs had the 1 phantom 404, website and ui-website were clean. Both fixes verified against the real app graphs; counterfactual tests fail when either guard is removed.
… masks literals Invariant 1 and the module-graph.js module-map line now state that transitiveDeps stops at .server.* like the gate (preloads are a subset of servable), and that the import scanner ignores import/export shown as example code inside a template literal.
vivek7405
left a comment
There was a problem hiding this comment.
Went through the correctness of both guards end to end. The mask is genuinely position-preserving (one char out per char in, newlines kept), so masked[m.index] lands on the same keyword byte in raw source even past the multi-byte ellipsis/arrow chars in this file, and I confirmed real imports after division, postfix ++, and regex literals still resolve while template-embedded and comment-embedded import text gets blanked at the keyword. The server-boundary stop sits after the skip check and the result push but before the queue push, so the .server.* URL stays preloadable while its imports are not walked, and it is a no-op for the two elision callers since they already carry serverFiles in their skip set. Both new tests fail when their own guard is pulled, which is what I wanted to see.
Two things I am explicitly leaving alone because they predate this change and the change only ever shrinks the phantom-edge set: a single-line untagged backtick literal containing an import, and a from-specifier sitting inside a block comment between a real export keyword and its real specifier, can still register a stray edge. Neither caused any of the four-app 404s and neither is made worse here. Last round was clean.
vivek7405
left a comment
There was a problem hiding this comment.
Second pass, focused on the one thing that worried me: the guard only checks the keyword position, not where the captured specifier lands. Walked the nasty cases. A real export keyword whose lazy from-gap binds a specifier inside a block comment or template does still miss-bind, but that is EXPORT_FROM_RE's lazy gap and it behaved identically before any mask existed, so this change neither fixes nor worsens it. Going the other way, I could not construct a valid import whose keyword offset gets blanked: the redactor is one-char-per-char so the offset always lands on the real keyword byte, and the word-boundary anchors keep the match off mid-token positions like reimport. Cache path is fine too, masked is only computed on a miss and matchAll always hands back a numeric index, and if it somehow did not the comparison fails toward keeping an edge, never dropping one. Clean.
|
How these were found, and the reusable audit Both bugs are the same class (a The live audit that surfaced them: fetch each app's HTML, pull every This is also why both fixes live in one PR rather than two: they are one invariant ("the preload set is a subset of the servable set"), violated in two different spots. Keeping the invariant stated once, in |
The unit layer already covers transitiveDeps + the scanner mask. This adds the higher layers the change affects: - Integration (test/ssr/ssr.test.js): ssrPage must not emit a preload for a server-only util reached THROUGH a .server file (the slugify.ts shape), while a real client dep is still preloaded. The existing test only covered the .server file itself, not its transitive dep. - E2E (test/e2e/e2e.test.mjs): probe every modulepreload href on the real blog and assert none 404 (the live symptom of #158/#159). - Smoke (test/examples/blog/smoke): same probe, browserless and fast.
vivek7405
left a comment
There was a problem hiding this comment.
Added the higher test layers and went back over them with a skeptic's eye, mostly worried the e2e and smoke probes could pass vacuously. They do not: each guards on preloads.length > 0 before probing, the same-origin filter drops the CDN preloads without emptying the set, and the gate returns a real 404 for a non-servable source URL rather than a redirect, so fetch following redirects cannot quietly turn a 404 into a 200. The href-extraction regexes match the actual link rel=modulepreload href= shape ssr.js emits. Integration test carries a positive assertion (the real client dep IS still preloaded) so an over-eager filter would fail it, not pass it. Fixture dirs are unique-suffixed and cleaned. Last round clean.
* chore: release @webjsdev/core 0.7.3 and @webjsdev/server 0.8.3 Release debt accumulated since the 0.7.2 / 0.8.2 release (#149): core 0.7.3 (patch): the two client-router fixes from #151 and #157 (JS-handled forms and links were hijacked by the router despite e.preventDefault). server 0.8.3 (patch): #161 (modulepreload hints no longer point at server-only or template-embedded files the auth gate 404s) and #156 (webjs check no longer leaks a git env var across worktrees). Both stay in a single minor line; all in-repo dependents pin ^0.7.0 / ^0.8.0, so no dependent range edits are needed. Lockfile regenerated. * chore: release @webjsdev/cli 0.10.0 (webjs vendor surface) cli 0.9.1 shipped without the `webjs vendor` command surface that #105 and #89 added (pin / unpin / list / audit / outdated / update, the --from multi-CDN selector, and --download caching). That is feature-level work, so a minor bump. create-webjs and webjsdev are thin shims that delegate to cli; widen their `@webjsdev/cli` range from ^0.9.0 to ^0.10.0 so the workspace keeps linking the local cli (a minor bump falls outside the old caret). They are not in the changelog auto-publish system, so they carry no changelog entry of their own. Lockfile regenerated.
The no-build page's modulepreload section said preloads cover "transitive dependencies" and only carved out .server.* files themselves, which read as the pre-#161 behavior (a plain util imported through a server action would be preloaded). #161 made the walk stop at the .server boundary, so those transitive server-only deps are excluded too. Document that the preload set is exactly the set the page fetches.
The no-build page's modulepreload section said preloads cover "transitive dependencies" and only carved out .server.* files themselves, which read as the pre-#161 behavior (a plain util imported through a server action would be preloaded). #161 made the walk stop at the .server boundary, so those transitive server-only deps are excluded too. Document that the preload set is exactly the set the page fetches.
Summary
Closes #158
Closes #159
The SSR preload emitter and the source-serving authorization gate walked the module graph differently, so the framework emitted
<link rel="modulepreload">hints for URLs it then 404s. Found by auditing all four in-repo dogfood apps for broken preloads./modules/posts/utils/slugify.ts,/modules/posts/types.ts,/modules/auth/types.ts.server.*boundary (#158)/app/docs/components/counter.tsimportshown as code inside a template literal (#159)What changed
transitiveDepsstops at.server.*(#158).reachableFromEntries(the auth gate) already stops at.server.{js,ts,mjs,mts}, because a client import of a server file resolves to an RPC / throw-at-load stub and the server file's own imports never reach the browser.transitiveDeps(which feeds the preload hints) did not, so it walked through a.server.tsinto its server-only deps and preloaded them. It now applies the sameSERVER_FILE_REstop, so a preload set is always a subset of the servable set. The.server.*file itself stays preloadable (its stub URL is fetched on demand), and a file reachable through both a server file and a real client path is still reached via the client path.Import scanner masks string/template content (#159).
parseFileran the import regexes on raw source, so animport/export … fromprinted as example code inside anhtml`` template (apackages/server/test/module-graph/module-graph.test.js):transitiveDepsstops at the.server.*boundary;buildModuleGraphignores import/export shown as code inside a template literal. Both fail when their guard is removed.test/ssr/ssr.test.js):ssrPagedoes not emit a preload for a server-only util reached THROUGH a.serverfile, while a real client dep still is. Counterfactually verified (fails against the pre-fixmodule-graph.js).test/e2e/e2e.test.mjs): probe every modulepreload href on the real blog, assert none 404. Suite 52/52.test/examples/blog/smoke/blog-smoke.test.js): same probe, browserless. Suite 6/6.Server suite 628/628. Dogfood, all four apps (prod mode): blog
/10 preloads / 0 broken (was 13/3); docs/docs/getting-started5/0 (was 6/1); website + ui-website 200, 0 broken. The 21 redpackages/core/test/**/browser/*.test.jsundernode --testare pre-existing wtr tests run under the wrong runner, unrelated to this change.Docs
packages/server/AGENTS.md: invariant 1 and themodule-graph.jsmodule-map line now state thattransitiveDepsshares the.server.*stop (preloads are a subset of servable) and that the scanner ignores imports shown inside template literals.