feat: reduce declarative template size#7498
feat: reduce declarative template size#7498janechu merged 21 commits intoreleases/fast-element-v3from
Conversation
Reduce declarative template entrypoint size by moving optional map and lifecycle support behind schema hooks, replacing the custom template element with a native publisher, and updating docs/fixtures. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Expose attributeMap and observerMap from extension subpaths, keep declarative re-exports, and support schema-driven usage with optional definition schemas. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds remaining optional export subpaths for binding, DOM, schema, templating, render, hydration, and node observation, while keeping Observable/observable together and controller/definition internals on the root entrypoint. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keep the public fast-element export paths flat while resolving package export types, test, and default targets to the original source and output locations. Also address review feedback by preserving explicit declarative schemas and avoiding shell-string API doc generation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keep the generated size tables focused on the previously tracked exports plus the requested attributeMap and observerMap subpath entries. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Re-export the runtime APIs that moved to subpaths from the rollup-only entries so the CDN bundle keeps its previous surface and size while the package root remains trimmed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
janechu
left a comment
There was a problem hiding this comment.
Reviewed the source-code changes (core runtime, components/hydration, declarative + observer-map-utilities, package exports, tests/fixtures, consumer updates).
Overall the refactor is well-executed:
- Flat subpath exports are consistent;
types-first ordering in the export map is correct; every new entry point has a matchingapi-extractor.<name>.json. - Hydration is properly isolated:
element-controller.tsandview.tsdo not import the new hydration runtime, preserving tree-shakability. - The shell→
execFilechange insites/website/scripts/generate-docs.cjscorrectly closes a real command-injection vector (noshell: true, args passed as array). - Changefiles accurately reflect the breaking nature of the path restructuring.
- Test fixture updates and the new
declarative-no-hydrationecosystem fixture are reasonable; bundle-size validation is appropriately deferred toSIZES.mdtracking.
One concrete issue worth addressing inline. I also looked carefully at deepMerge in observer-map-utilities.ts (the nestedChanged === false branch is correct because nextTarget is a copy of targetValue and deepEqual short-circuits earlier when source/target are equivalent), the addPropertiesToAContext/anyOf access in components/schema.ts (the splitPath.length > 1 and else if (childRef) paths are mutually exclusive, and schema.properties[splitPath[0]] is initialized to {} immediately above), and the polyfill removal (no remaining importers found). Those checked out.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
janechu
left a comment
There was a problem hiding this comment.
Review summary (fleet review)
Thanks for this substantial cleanup! The export-map flatten and the schema-transform tree-shaking are well executed overall — flat subpaths resolve cleanly (no stale nested imports remain in packages/, sites/, examples/, or .github/), sideEffects: false is genuinely defensible given the new modules are pure re-exports, and the 600+ line drop in declarative/utilities.ts is fully accounted for (functions relocated to observer-map-utilities.ts, template-bridge.ts, etc.).
I reviewed in four parallel passes (exports, schema/observer-map refactor, template-bridge/hydration, and the execFile/docs/change-file pass). Findings below; inline comments cover the higher-impact ones where the line falls in the diff.
Headline issues
-
The advertised
execFilesecurity fix is not actually present. The PR description says shell-string command generation was replaced by argument-basedexecFileusage, but the final commit8a2b3218f "Restrict fast-element path exports"revertedsites/website/scripts/generate-docs.cjsback toexec(\api-documenter markdown -i ${tempAPIDir} -o ${markdownAPIDir}`)at lines 216 and 233. The net diff for that file is empty. Earlier commits (8acba4a,2cff100) had therunAPIDocumenter()helper usingexecFile(cmd, ["markdown", "-i", input, "-o", output])— that helper needs to be restored before merge.markdownAPIDiris built fromprocess.argv[2]` (line 13) without sanitisation, so this is a real injection surface, not just a hardening preference. -
<f-template>.publishTemplaterejects legitimate nested<template>content —getElementsByTagName("template")walks all descendants, so any user template that contains nested<template>elements (declarative shadow DOM, sub-template patterns, even content like a<table>row template) trips the multi-template guard. Inline comment ontemplate.ts. -
ObserverMap${prop}Changedwrapping is not idempotent — re-running the immediate-execution path against the same prototype nests wrappers indefinitely. Important footgun for the new transform-driven flow that may apply on republish or hot-reload. Inline comment onobserver-map.ts. -
ObserverMap.applyConfigToSchemamutates a shared schema in place with$observe: falsestamps. A caller that reuses oneSchemainstance across elements, or callsobserverMap()twice with progressively-permissive configs, can never re-include previously-excluded nodes (stampObserveFalseonly writesfalse). Now relevant because schemas are commonly shared viadefinition.schema. -
Removed deferred-template guard in
ElementController.connect(theif (!this.template && definition.templateOptions === TemplateOptions.deferAndHydrate) return;early-return). With the new template-bridge flow, async-resolved templates makeconnect()proceed withtemplate === null, which permanently resolves the throwaway controller's_resolvePrerendered/_resolveHydratedtofalse— even though the replacement controller installed by thetemplate-change subscription will then hydrate correctly. Consumers that read$fastController.isPrerenderedbefore the second controller arrives observe a misleadingfalse. Lifecycle hooks also fire twice.
Medium issues
attributeMap()silently no-ops when called without a schema and without a template resolver, whileobserverMap()throws at the same location (observer-map.ts:109). Inconsistent failure modes — please mirror.HydrationTracker.startedlatches totruepermanently (hydration-tracker.ts:33). Subsequent SSR batches (per-route hydration, dynamically inserted SSR fragments) silently bypasshydrationStarted/hydrationComplete. Either reset on completion or document single-batch.ensureHydrationRuntimemutatesViewTemplate.prototypeglobally and irreversibly viaObject.defineProperty(..., { configurable: false })(hydration/runtime.ts:22). Once any code path callsenableHydration()in a process, everyViewTemplatebecomes hydratable everywhere — including parallel Playwright workers sharing a context. No teardown / per-instance scoping.MutationObserverinstalled byobserveLateAttributesis never disconnected (element-controller.ts:644). It keeps observing while the element is detached and there's no disposal hook onElementController.forCustomElementre-subscribes a newtemplateandshadowOptionslistener on every override invocation (element-controller.ts:865, 875). This was a pre-existing concern in v3, but the new declarative-template hand-off makes theoverride = truerecursion routine, so the leak path is now hit in normal flow rather than rare hot-swap.- Change-file review:
change/@microsoft-fast-element-22265526-…jsoncorrectly usesmajor. Howeverchange/@microsoft-fast-router-de58ce36-…jsonis the only file in this PR usingprerelease— verify that's a valid type for thereleases/fast-element-v3branch inbeachball.config.js; everything else usesmajor/minor/none.
Doc inconsistencies
MIGRATION.md:225–227references@microsoft/fast-element/install-hydratable-view-templates.jsas "still available", but that path is not in the export map (only./hydration.jsexists). Either add the export or delete the sentence.MIGRATION.md:162–163prose says "attributeMap()andobserverMap()are imported from@microsoft/fast-elementfor declarative templates" — but the surrounding code samples (and the actual export map) require/attribute-map.jsand/observer-map.js. Prose contradicts examples.MIGRATION.md:65andmigration-guide.md:145mix one hydration row pointing at/hydration.jsinto a table whose other rows are all root-package imports. Worth splitting or footnoting.- A few stale references to
TemplateElement.options()/TemplateElement.config()inDECLARATIVE_DESIGN.mdandDECLARATIVE_SCHEMA_OBSERVER_MAP.md— names removed in this PR.
Test coverage gaps (template-bridge.pw.spec.ts)
The new 649-line spec is solid, but I'd add:
Message.noTemplateProvided(an<f-template>with no<template>child)Message.moreThanOneTemplateProvided(multiple direct<template>children in one<f-template>)- A nested-
<template>inside the user template — this would surface issue (2) above - A
repeat/whendirective inside a declarative template - An end-to-end hydration test (declarative template + SSR markers +
enableHydration+ asserthydrationStage === 'hydrated') <slot>elements inside a declarative template- Race:
FASTElement.define()resolving before the<f-template>is parsed and connected (currently weakly covered by "waits for definition-first markup")
The unit-level observer-map.spec.ts only exercises the immediate-execution path. The transform/lazy/merge path that is the central change of this PR is only covered indirectly by the Playwright spec. A unit test that asserts (a) deterministic transform ordering under attributeMap + observerMap, (b) idempotency under double-application, (c) definition.schema = userSchema is preserved by reference when declarativeTemplate() augments it, would catch regressions much faster.
Smaller observations (non-blocking)
package.json: thetestcondition is set on most flat subpaths but missing on./debug.js,./utilities.js,./state.js,./context.js,./di.js,./dom-policy.js. Either add for symmetry or document the rule.test/tsconfig.jsonpathsblock is missing several flat subpaths (binding.js,dom.js,schema.js,templating.js,render.js, …). A new test importing those will silently bypass the path map.src/arrays.tsre-exportsSubscriberSetas atype-only export, but it's a runtime class. WithverbatimModuleSyntax, consumers cannot construct it through this subpath. If intentional (runtime is via./observable.js), drop the export from./arrays.js.dist/declarative/definition-options.*artifacts appear to exist butsetDefinitionSchemaTransformis not surfaced as a public subpath. Either expose it (if third-party schema transforms are supported) or remove the stale dist files.template.ts:48uses barenew Error(...)for a definition-conflict while the rest of the module usesFAST.error(Message.*)— adding aMessageenum entry keeps debug-stripped builds usable.template.ts:167attributeChangedCallbackhas parameters namedpreviousName/nextNamebut they are attribute values, not names.
I'll leave the inline comments below for the higher-impact items where the GitHub API supports inline placement on the diff.
Scope correctionA few items in my review summary above turned out to be in code that is already on
The rest of the findings in the summary above (the |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Pull Request
📖 Description
Reduces the declarative template entrypoint size by making optional attribute/observer map and hydration lifecycle support tree-shakable through schema transforms, replacing the custom
TemplateElementFASTElement with a native<f-template>publisher, and updating declarative docs, generated API docs, fixtures, benchmarks, size docs, and the change file.Optional helpers now use dedicated flat
@microsoft/fast-elementsubpaths and are no longer exported from the root entrypoint or the old nested helper paths. The package export map keeps the public paths flat while targeting the actual source and output file locations fortypes,test, anddefault.Examples of the flat subpaths:
@microsoft/fast-element/attribute-map.js@microsoft/fast-element/observer-map.js@microsoft/fast-element/children.js@microsoft/fast-element/ref.js@microsoft/fast-element/slotted.js@microsoft/fast-element/when.js@microsoft/fast-element/repeat.js@microsoft/fast-element/node-observation.js@microsoft/fast-element/two-way.js@microsoft/fast-element/signal.js@microsoft/fast-element/declarative-utilities.jsAdditional dedicated paths include
updates.js,observable.js,attr.js,volatile.js,css.js,html.js,array-observer.js,binding.js,dom.js,schema.js,templating.js,render.js, andhydration.js.FASTElementDefinition.schemais optional; declarative templates assign or augment it during template resolution, and non-declarative/manual schema users can useobserverMap({ schema })or provide a schema on the definition. Explicitly supplied schemas are preserved.This also addresses PR review feedback by replacing shell-string API document generation with argument-based
execFileusage.📑 Test Plan
npm run build:tsc -w @microsoft/fast-element -- --pretty falsenpm run doc:exports -w @microsoft/fast-elementnpm run doc:exports:ci -w @microsoft/fast-elementnpm run build:sizes -w @microsoft/fast-elementnpm run prebuild -w @microsoft/fast-sitenpm run biome:checknpm run checkchangenode -c sites/website/scripts/generate-docs.cjsgit diff --check✅ Checklist
General
$ npm run change