Conversation
Render shadowroot-prefixed attributes from f-template onto declarative template output while preserving the existing open defaults. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
5725fe7 to
d5569e9
Compare
There was a problem hiding this comment.
The parsing/escaping path is solid and the test coverage for the common cases is good. A few concerns left as inline comments. The most notable is an asymmetric shadowrootmode ↔ shadowroot mirroring: when a user specifies only shadowroot="closed" (no shadowrootmode), the output is <template shadowrootmode="open" shadowroot="closed">, which gives modern browsers "open" and legacy ones "closed". I verified this by running the new logic against that input. Other comments cover a boolean-vs-empty-value distinction, a public Rust API gap, missing test coverage for the asymmetric case, and a small JSON parsing nit.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
janechu
left a comment
There was a problem hiding this comment.
Re-review of e674d99 — all six previous comments are addressed cleanly:
- Asymmetric mirror ✅ — new
explicit_shadowroot_attr_valuehelper plus theor_else/orchain mirrorsshadowroot → shadowrootmodeas well, so<f-template shadowroot="closed">now yieldsshadowrootmode="closed" shadowroot="closed". New testtest_f_template_legacy_shadowroot_value_mirrors_to_mode+ fixtureshadowroot-legacy-closed.htmlpin it down. - Boolean vs empty-string ✅ —
explicit_shadowroot_attr_valuefilters empty strings out, soshadowrootmode=""is now treated identically to the boolean form. Four new fixtures + tests cover the empty cases (mode-only, shadowroot-only, mode-empty + shadowroot="closed", mode="closed" + shadowroot-empty). - Public Rust API gap ✅ — added
add_template_with_shadowroot_attrsplus rustdoc onfrom_templatesandadd_templatepointing users there. Unit testtest_add_template_with_shadowroot_attrs_preserves_attrsadded. - Test coverage gap ✅ — covered by item #1.
- JSON
nullshadowrootAttributes ✅ —Some(json::JsonValue::Null) | None => Vec::new()arm added; wasm-test added (note: gated totarget_arch = "wasm32", which matches the rest ofwasm.rs). - Order-coupled JS assertion ✅ — refactored to extract the
<template …>tag once and assert each attribute's presence individually.
Verified with cargo test (65 unit + 60 integration tests pass) and npm run test:node -w @microsoft/fast-build (30/30 pass). Also re-ran the truth table for build_shadowroot_template_attrs — explicit conflicts like shadowrootmode="open" shadowroot="closed" are still preserved verbatim, which is the right call.
One small follow-up left as an inline comment: the user-facing docs still only describe the one-way mode → shadowroot mirror.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Re-review of 0307207 — doc lag is fully addressed across all four files (crates/microsoft-fast-build/{DESIGN,README}.md, packages/fast-build/{DESIGN,README}.md). The new wording is consistent across locations and accurately describes the implemented behavior:
- Neither attribute non-empty → defaults to
shadowrootmode="open" shadowroot="open" - Exactly one has a non-empty value → mirrored to the other
- Both have explicit non-empty values → preserved as authored
Cross-checked against the truth table of build_shadowroot_template_attrs/explicit_shadowroot_attr_value — all four cases match.
No further blocking issues.
# Pull Request ## 📖 Description Brings `releases/fast-element-v3` up to date with `main`. Cherry-picks every commit landed on `main` after PR #7477 was merged (the last v3↔main sync) and adds an adaptation commit so the changes work with the v3 architecture (where `@microsoft/fast-html` was merged into `@microsoft/fast-element` under `src/declarative/`). Cherry-picked (in chronological order): - #7479 — feat: change default attribute-name-strategy from none to camelCase - #7494 — chore(deps): bump liquidjs 10.25.6 → 10.25.7 - #7501 — chore: migrate TypeScript compilation to `typescript/native-preview` (`tsgo`) - #7502 — feat: allow `fast-build` rendering without state - #7503 — docs: per-package documentation reorg (DESIGN.md/README.md and `docs/architecture/*`) - #7504 — feat: propagate shadowroot attributes - #7505 — chore: bump packages after failed publish (`fast-build` → 0.5.0) - #7469 — feat: add build utilities and SSR renderer to `@microsoft/fast-test-harness` - #7506 — chore: prepare `@microsoft/fast-test-harness` package for publishing The "applying package updates" beachball commit (`aad6f5481`) was intentionally skipped — v3 maintains its own versioning track (`fast-element@3.0.0-rc.1`, `fast-test-harness@0.0.1`) and runs its own publish. Adaptation commit highlights: - Adds a new `./declarative-syntax.js` subpath export to `@microsoft/fast-element` so tooling consumers (e.g. `fast-test-harness` build utilities) can access declarative HTML syntax constants from fast-element directly. Mirrors the existing `./declarative-utilities.js` subpath. - Re-points `fast-test-harness/src/build/generate-templates.ts` and `generate-webui-templates.ts` from `@microsoft/fast-html/syntax.js` to the new subpath. - Replaces the removed `RenderableFASTElement(...).defineAsync({ templateOptions })` pattern with v3's `Element.define()` in test fixtures and the README example. - Drops `@microsoft/fast-html` from the harness Vite optimizer exclude list. - Excludes `*.spec.ts` from the fast-element TS build (the declarative `.spec.ts` files import `@playwright/test`, whose `.d.ts` files are incompatible with `tsgo`'s stricter parser; these specs are still type-checked when run via the declarative Playwright config). - Adapted hydration marker handling in `crates/microsoft-fast-build/src/node.rs` for #7502 (kept v3's `inject_count_marker` / `data-fe="N"` style while merging in main's `Option<&mut HydrationScope>` flow). - All `packages/fast-html/*` paths from cherry-picks were dropped (the package no longer exists on v3); related `change/@microsoft-fast-html-*.json` files were either converted to `@microsoft-fast-element-*.json` or dropped where they no longer apply. ## 📑 Test Plan Run from the monorepo root on this branch: - `npm run build` — all 6 packages build successfully (fast-build, fast-element, fast-router, fast-test-harness, fast-site, fast-todo-app-example). - `@microsoft/fast-element` — 1410 chromium playwright tests + 176 declarative chromium tests pass. - `@microsoft/fast-test-harness` — 96 node unit tests + 43 chromium playwright tests pass. - `npm run biome:check` — clean. - `npm run checkchange` — passes. ## ✅ Checklist ### General - [x] I have included a change request file using `$ npm run change` - [ ] I have added tests for my changes. - [x] I have tested my changes. - [x] I have updated the project documentation to reflect my changes. - [x] I have read the [CONTRIBUTING](https://github.com/microsoft/fast/blob/main/CONTRIBUTING.md) documentation and followed the [standards](https://github.com/microsoft/fast/blob/main/CODE_OF_CONDUCT.md#our-standards) for this project.
Pull Request
📖 Description
shadowroot*attributes fromf-templateonto rendered declarative shadow DOM<template>elements.shadowrootmode="open"andshadowroot="open"behavior when no mode is provided.👩💻 Reviewer Notes
Please focus review on the shadowroot attribute normalization/escaping path shared between the Rust renderer and the JS CLI configuration bridge.
📑 Test Plan
Passed:
git diff --checkcargo test --manifest-path crates/microsoft-fast-build/Cargo.tomlnpm run build -w @microsoft/fast-buildnpm run test:node -w @microsoft/fast-buildnpm run buildnpm run biome:checknpm run checkchange✅ Checklist
General
$ npm run change