Add build output manifest for static caching#262
Conversation
Coverage Report for CI Build 27524398777Coverage increased (+0.3%) to 92.204%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
31311d7 to
620a4bc
Compare
fb90546 to
e8c79b8
Compare
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
e8c79b8 to
f665681
Compare
a3079c4 to
260af7a
Compare
0fe8310 to
714e90d
Compare
| await writeFile(join(dest, 'domstack-esbuild-meta.json'), JSON.stringify(buildResults.metafile, null, ' ')) | ||
| const serviceWorkerBuildOpts = createServiceWorkerBuildOpts({ buildOpts: extendedBuildOpts, src, siteData }) | ||
| const serviceWorkerBuildResults = serviceWorkerBuildOpts | ||
| ? await esbuild.build(serviceWorkerBuildOpts) |
There was a problem hiding this comment.
Does the service worker have its own build?
| // @ts-ignore This is fine | ||
| buildOpts: extendedBuildOpts, | ||
| outputs, | ||
| }, |
There was a problem hiding this comment.
Does it make sense to make report a single key object. Why not just return outputs as results.
| * @param {...(esbuild.BuildResult | undefined)} results | ||
| * @returns {esbuild.BuildResult} | ||
| */ | ||
| function mergeBuildResults (...results) { |
There was a problem hiding this comment.
Is this done efficiently? Would a reduce or for look get this down tone a single iteration?
|
|
||
| return { | ||
| inputs: Object.assign({}, ...metafiles.map(metafile => metafile.inputs)), | ||
| outputs: Object.assign({}, ...metafiles.map(metafile => metafile.outputs)), |
There was a problem hiding this comment.
Why object adding instead of just spreads
| * @param {boolean} params.includeMetafileRecord | ||
| * @returns {DomstackManifestRecord[]} | ||
| */ | ||
| export function createEsbuildOutputRecords ({ src, dest, siteData, buildResults, includeMetafileRecord }) { |
There was a problem hiding this comment.
Need to audit for efficiency.
| * @param {PageData<any, any, any>} page | ||
| */ | ||
| function extractPrecacheVars (page) { | ||
| /** @type {{ precache?: unknown, offline?: unknown }} */ |
There was a problem hiding this comment.
What is this? This seems like a peak concern sneaking in to what really should be a manifest collecting feature.
| template, | ||
| outputRecords, | ||
| }) | ||
| } else if ( |
There was a problem hiding this comment.
Are we using exhaustive type narrowing here.
| * @param {string} value | ||
| */ | ||
| function toPosix (value) { | ||
| return value.split(sep).join('/') |
There was a problem hiding this comment.
There is a version of this that's exported somewhere. Make sure this isn't a double define.
| * @param {string} dest | ||
| * @param {string} filepath | ||
| */ | ||
| function assertInsideDest (dest, filepath) { |
There was a problem hiding this comment.
Should be a generic helper lib.
| * pages: Awaited<ReturnType<typeof pageWriter>>[] | ||
| * templates: Awaited<ReturnType<typeof templateBuilder>>[] | ||
| * outputs: DomstackManifestRecord[] | ||
| * }} PageBuilderReport |
There was a problem hiding this comment.
Why did this collapse. Double check we should just be dealing with what's in outputs.
| try { | ||
| const report = await copy(getCopyGlob(src), dest, ...(opts?.ignore ? [{ ignore: opts.ignore }] : [])) | ||
| results.report = report | ||
| results.report.outputs = createCopiedDomstackManifestRecords({ |
There was a problem hiding this comment.
Double check what this is doing.
|
|
||
| // These schema objects are the source of truth for both runtime manifest validation and | ||
| // the public TypeScript/JSDoc types derived below with json-schema-to-ts. | ||
| /** @satisfies {JSONSchema} */ |
There was a problem hiding this comment.
I don't think this syntax is right this needs to be inline with the type const.
| ], | ||
| }) | ||
|
|
||
| /** @satisfies {JSONSchema} */ |
There was a problem hiding this comment.
Same. Bad satisfies syntax.
| precache: { | ||
| description: 'Application-defined page precache policy metadata. Domstack records this value but does not interpret it.', | ||
| }, | ||
| offline: { |
There was a problem hiding this comment.
Seems odd to predefne this capture. Maybe let users configure which keys to capture.
| additionalProperties: false, | ||
| }) | ||
|
|
||
| /** @satisfies {JSONSchema} */ |
There was a problem hiding this comment.
Every schema has bad syntax I'm fairly certain.
| * A build step writes these as it emits files. Reconciliation turns them into | ||
| * revisioned manifest entries. | ||
| * | ||
| * @typedef {object} DomstackManifestRecord |
There was a problem hiding this comment.
These should have descriptions right?
| // TODO: If a concrete client needs Workbox integration, consider adding an | ||
| // optional derived artifact that projects manifest entries to Workbox's | ||
| // `{ url, revision }` precache shape. Keep the domstack manifest as the richer | ||
| // source of truth until that use case can validate the exact API. |
There was a problem hiding this comment.
When this is done also confirm we support the right lifecycle hook for workbox. I think it relies on full code gen for the service worker.
| // `{ url, revision }` precache shape. Keep the domstack manifest as the richer | ||
| // source of truth until that use case can validate the exact API. | ||
|
|
||
| const KIND_PRIORITY = new Map([ |
| * @returns {Promise<DomstackManifest>} | ||
| */ | ||
| export async function buildDomstackManifest ({ dest, records = [], entries: existingEntries = [], options = {} }) { | ||
| /** @type {Map<string, DomstackManifestRecord & { url: string, revision: string | null, bytes: number | null }>} */ |
There was a problem hiding this comment.
Why is this type ugly. Why don't we have a clean type to use here.
| setEntry(entryMap, entry) | ||
| } | ||
|
|
||
| entryMap.delete(toPosix(options.filename ?? DEFAULT_DOMSTACK_MANIFEST_FILENAME)) |
There was a problem hiding this comment.
Should we be deleting records?
|
|
||
| entryMap.delete(toPosix(options.filename ?? DEFAULT_DOMSTACK_MANIFEST_FILENAME)) | ||
|
|
||
| let finalEntries = Array.from(entryMap.values()) |
There was a problem hiding this comment.
We should try to make this all happen in a single pass rather than multiple iterations.
| * Create a normalized record for a file inside dest. | ||
| * | ||
| * @param {object} params | ||
| * @param {string} params.dest |
| kind, | ||
| url: url ?? outputRelnameToUrl(normalizedOutputRelname), | ||
| ...(sourceRelname ? { sourceRelname: toPosix(sourceRelname) } : {}), | ||
| ...(entryPoint ? { entryPoint: normalizeEntryPoint(entryPoint) } : {}), |
| */ | ||
| export function createCopiedDomstackManifestRecords ({ src, dest, report, kind }) { | ||
| return extractCopiedFiles(report).map(copiedFile => { | ||
| const filepath = resolve(copiedFile.output) |
There was a problem hiding this comment.
I wonder if this would better live near cpx2 builder and instead have it just conform to a type over in that file.
| */ | ||
| export function classifyEsbuildOutput ({ outputRelname, entryPoint, workerOutputRelnames, serviceWorkerOutputRelname }) { | ||
| const ext = extname(outputRelname) | ||
|
|
There was a problem hiding this comment.
Same here. Closer to es build builder and use a type
| * @param {DomstackManifest} domstackManifest | ||
| * @param {string} [filename] | ||
| */ | ||
| export async function writeDomstackManifest (dest, domstackManifest, filename = DEFAULT_DOMSTACK_MANIFEST_FILENAME) { |
There was a problem hiding this comment.
We don't have a generic version of this
| /** @type {DomstackManifestOptions} */ | ||
| const options = { | ||
| exclude: [ | ||
| ...((Array.isArray(domstackManifestOpts.exclude) ? domstackManifestOpts.exclude : [])), |
| * @param {string} relname | ||
| */ | ||
| export function outputRelnameToUrl (relname) { | ||
| const posixRelname = toPosix(relname) |
There was a problem hiding this comment.
That we didn't define this elsewhere.
| * @param {string} value | ||
| */ | ||
| export function toPosix (value) { | ||
| return value.split(sep).join('/') |
There was a problem hiding this comment.
Generic helpers should be inserted generic file location
| * @param {string[]} exclude | ||
| */ | ||
| function applyExclude (entries, exclude) { | ||
| if (exclude.length === 0) return entries |
There was a problem hiding this comment.
Why is manifest dealing with this. Shouldnt it be totally downstream from ignores?
Summary
This adds an unstable-preview domstack output manifest for static app caching and pairs it with first-class site service-worker support.
Warning
The domstack manifest,
domstack-manifest.settings.*, first-classservice-worker.*entries, and related browserprocess.env.DOMSTACK_*defines are preview APIs. Their names, option shapes, manifest schema, generated output, and runtime semantics may change outside of a major version while the API is validated with real PWA use cases. Consumers should pin@domstack/staticto an exact version before relying on this contract.The goal is to let a domstack app build a fully static client, then have its own service worker consume a normalized, revisioned list of emitted files. Domstack provides the build facts; the application still owns service-worker registration, update UX, route filtering, offline behavior, and cache policy.
What Changed
domstack-manifest.json, written by one-shot builds by default.results.domstackManifestfrom programmatic builds.report.outputsrecords.$schemaURL.@domstack/static/types.js/#typesusingjson-schema-to-ts.domstack-manifest.settings.{js,mjs,cjs,ts,mts,cts}for manifest filtering policy.service-worker.{js,mjs,cjs,ts,mts,cts}anywhere undersrcbuilds to stable root/service-worker.js..npmignorewith apackage.json#filesallowlist.New Preview APIs
Build Results
results.domstackManifestis still returned when writing the JSON file is disabled withdomstackManifest: false. Watch mode does not return a domstack manifest.CLI Options
--customDomstackManifestNamechanges the written manifest filename from the defaultdomstack-manifest.json.--noDomstackManifestdisables writing the JSON file.--serveruns a one-shot build and then serves the output without watch mode, which is useful for testing service-worker cache lifecycle behavior with a manifest-enabled build.Programmatic Options
Supported shape:
domstack-manifest.settings.*Apps can configure the generated manifest from a dedicated settings file anywhere under
src:The default export can be an object, a sync function, or an async function returning an object:
Filtering affects both the written
domstack-manifest.jsonandresults.domstackManifest, and it affectsmanifest.versionbecause versioning is based on final cache-relevant entries.Service Worker Support
Domstack reserves one site service-worker source filename:
The source may live anywhere under
src, but only one is allowed. Multiple matches fail withDOM_STACK_ERROR_DUPLICATE_SERVICE_WORKER.The service worker is bundled through esbuild and emitted with a stable root output path:
Domstack does not inject registration into the default layout. Registration timing, update prompts, local-development opt-outs, poisoned-cache recovery, and offline route behavior remain application policy.
Browser Defines
process.env.DOMSTACK_MANIFEST_URL/domstack-manifest.jsonprocess.env.DOMSTACK_MANIFEST_ENABLED"true"for one-shot builds that write the manifest,"false"when disabled or in watch modeprocess.env.DOMSTACK_SERVICE_WORKER_URL/service-worker.js, or""when absentprocess.env.DOMSTACK_SERVICE_WORKER_SCOPE/, or""when absentCompatibility Notes
copyis now the output kind for files copied by the generic copy step.domstack-manifest.jsonor returnresults.domstackManifest; it does still build and rebundle the site service worker.Testing
npm testnpm run test:tscnpm run test:neostandardnpm run build:declarationnpm run build:schemanode --test lib/identify-pages.test.js test-cases/general-features/index.test.js test-cases/type-exports/index.test.ts test-cases/watch/index.test.js test-cases/template-output-escape/index.test.jsgit diff --check