Skip to content

Regenerate Admin component definitions and docs - #4689

Merged
kyledurand merged 5 commits into
2026-10-rcfrom
cx-regenerate-admin-ui-docs
Sep 14, 2026
Merged

kyledurand merged 5 commits into
2026-10-rcfrom
cx-regenerate-admin-ui-docs

Conversation

@davejcameron

Copy link
Copy Markdown
Contributor

Background

The Admin components.d.ts aggregate had been generated for a subset of components and then patched by hand, so it had drifted from the components as shipped: AppNav, EmptyState, Number, PressButton and ScrollBox were missing entirely, and internal generator scaffolding had leaked in.

Solution

Regenerated the definitions for the full component set, with no component filter, and rebuilt the 2026-10-rc docs payload. Definitions move from Polaris 2.23.0 to 2.26.0: shared.d.ts gains ProgressProps and generalises the field documentation inherited by Checkbox, Choice, ChoiceList, Option and Switch; Section documents an Icon in its graphic slot and Tooltip among its accessories.

Everything here is generated output.

🎩

  • yarn lint and yarn type-check pass.

Checklist

  • I have 🎩'd these changes
  • I have updated relevant documentation

@davejcameron
davejcameron requested a review from a team as a code owner September 11, 2026 20:27
The components.d.ts aggregate had been generated for a subset of
components and then patched by hand, so it had drifted from the
components as shipped: AppNav, EmptyState, Number, PressButton and
ScrollBox were absent entirely, and internal generator scaffolding
(AddedContext, BaseClass, ContextRequestEvent) had leaked in where the
shared PolarisCustomElement base class belongs.

Regenerated for the full component set, then rebuilt the 2026-10-rc docs
payload. Definitions move from Polaris 2.23.0 to 2.26.0: shared.d.ts
picks up ProgressProps and generalises the field documentation inherited
by Checkbox, Choice, ChoiceList, Option and Switch, and Section
documents an Icon in its graphic slot and Tooltip among its accessories.

yarn lint and yarn type-check pass.
@davejcameron
davejcameron force-pushed the cx-regenerate-admin-ui-docs branch from 763912e to e54c7cc Compare September 11, 2026 20:28

@kyledurand kyledurand left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like nothing changed except for some wording and the addition of progress component 👍

@jas7457

jas7457 commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Heads up before this merges — the three generated_docs_data_v2.json files aren't just picking up wording + the Progress component. Each one grows from ~1.48 MB to ~10.5 MB (≈7×), and that's what the +183,699 / −12,043 diff mostly is.

The cause: every Admin component's members list in the docs payload now includes the entire native DOM prototype surface (HTMLElement/Element/Node), flattened in and marked public. So <s-avatar> etc. now "document" props like accessKey, className, style, innerHTML, onclick, querySelectorAll, ELEMENT_NODE, every aria* reflector and every on* handler — ~320 bogus members per component, across 66 components.

In the base file these didn't exist (Avatar had 4 real members: initials, src, size, alt; now it has 325). This looks like the base-class DOM-flatten guard drifting during regeneration, not intended output — the shopify.dev API reference would render hundreds of native DOM members on every component.

The .d.ts changes themselves look fine (they use class extends, so DOM members stay inherited, not flattened), and the intended additions (Number, EmptyState, AppNav, Progress, the field-wording generalizations, Section's Icon/Tooltip) are all present. It's just the docs JSON that needs a clean regen.

Technical detail for a fix (AI-friendly)

Symptom

  • packages/ui-extensions/docs/surfaces/admin/generated/{admin_extensions/2026-10-rc,app_home,app_home_ui_extension/2026-10-rc}/generated_docs_data_v2.json each go 1,480,849 → 10,522,689 bytes. All three are byte-identical to each other on both sides (same blob SHA), so it's one logical payload mirrored 3×.
  • Structure is data[ComponentName]["src/surfaces/admin/components.ts"].members (a list). Per-component member counts jump ~4–24 → ~325–345.

Root cause

  • The generator walked the class heritage (Avatar extends PolarisCustomElement extends PreactCustomElement extends HTMLElement) and flattened the full HTMLElement/Element/Node member set into each component's members array, instead of stopping at the PolarisCustomElement base. This is the "base-class DOM-flatten guard drift" gotcha.
  • The leaked members are emitted with filePath: "src/surfaces/admin/components.ts" and, critically, without isPrivate: true, so they count as public docs. Net-new public members per component ≈ 321, e.g. accessKey, accessKeyLabel, ariaLabel+all aria*, attachShadow, className, classList, dataset, id, innerHTML, outerHTML, style, tabIndex, title, slot, role, part, popover, querySelector(All), getBoundingClientRect, scroll*, append/prepend/remove/replaceWith, all on* handlers, and the Node constants (ELEMENT_NODE, DOCUMENT_POSITION_*, etc.).
  • Some inherited lifecycle/internal members did get isPrivate: true (setAttribute, updateComplete, click, and symbol members like __@shadowRootSymbol@1816, __@flushRenderSymbol@1817), so the private filter partly fired — but the bulk of the DOM surface slipped through as public.

Fix direction

  • Regenerate with the base-class DOM-flatten guard in place so member collection stops at PolarisCustomElement and does not descend into HTMLElement/Element/Node. Also ensure hidden/private lifecycle + symbol members are excluded.

How to verify the corrected payload

  • data["Avatar"]["src/surfaces/admin/components.ts"].members back down to its real props (~4).
  • Grep the payload for "ELEMENT_NODE", "onwebkitanimationend", "accessKey", "querySelectorAll" → should return nothing.
  • File size back in the ~1.5 MB range; the diff should be small (new components + Progress + wording only), not 7×.
  • No net-new public members whose filePath is src/surfaces/admin/components.ts beyond the actual component props/slots.

posted by an AI agent on Jason's behalf

@jas7457 jas7457 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes — the regenerated generated_docs_data_v2.json payloads have an unintended regression: each Admin component's members list now includes the full native DOM surface (HTMLElement/Element/Node) as public members (~320 bogus members × 66 components), ballooning each file from ~1.48 MB to ~10.5 MB. This looks like base-class DOM-flatten guard drift during regeneration, not intended output. Details and a fix/verification path in my inline comment: #4689 (comment)

The .d.ts changes and the intended additions (Number, EmptyState, AppNav, Progress, wording, Section Icon/Tooltip) look good — just the docs JSON needs a clean regen.

posted by an AI agent on Jason's behalf

The admin docs generator neutralizes the web-component base class before
running generate-docs so the entire HTMLElement/Element/Node DOM surface
is not flattened into every component's documented members. The guard
only rewrote the `typeof globalThis.HTMLElement` form; the definitions
build now emits the base as an object literal
(`declare const BaseClass: { new (): HTMLElement; prototype: HTMLElement }`),
which the guard missed, silently leaking ~320 public DOM members into
every component and ballooning the payload ~7x.

- Broaden the neutralization to also collapse the object-literal base
  form to `any` (name- and whitespace-agnostic).
- Add assertNoDomSurfaceLeak: after generation, fail the build loudly if
  any public admin component member is a native DOM member. A future
  base-class form change now errors instead of silently shipping a bloated
  payload.
Re-ran `yarn docs:admin 2026-10-rc` with the hardened generator. The
three generated_docs_data_v2.json payloads no longer carry the flattened
HTMLElement/Element/Node member surface on every component, so each drops
from ~10.5 MB back to ~1.7 MB. The intended additions (Number, EmptyState,
AppNav, Progress) and field-wording changes are retained.

@jas7457 jas7457 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking better. Having Kyle also run an agent review to make sure things are good now before merging.

@kyledurand kyledurand left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from Kyle's agent review

The native-DOM payload regression is fixed at this head. I reproduced the checked-in docs byte-for-byte (1,683,007 bytes per copy); omitting neutralization produced about 10.52 MB and the new assertion correctly rejected all 65 public component classes. No runtime-JS bundle increase is introduced here. Exported per-component declarations grow only 801,198 → 803,029 bytes; the larger increases are in the docs-input aggregate and docs JSON.

I've left inline comments for the remaining docs correction, changeset wording, failure cleanup, and measured opportunities to reduce metadata weight. Generator issues that already affect exported types are explicitly marked as pre-existing follow-ups, not new runtime blockers for this PR. Generated declaration issues should be fixed in the producer and regenerated, rather than patched by hand here.

Additional follow-ups outside the changed lines:

  • Shared aliases: components/Number.d.ts:11 imports TextProps, but shared.d.ts declares TextProps$1. With real dependencies and TypeScript 5.9.3, NumberProps['tone'] resolves to any and accepts an invalid tone. This predates this PR; by contrast, this regeneration fixes ProgressProps['max'] to resolve to number. A producer test that resolves every per-component import against shared exports would catch this class of bug.
  • Consumer install weight: ts-morph is a production dependency although its only code importer is the Loom build helper buildTargetDts.ts. Moving it to devDependencies avoids approximately 14 MB unpacked in a fresh dependency installation (incremental savings depend on deduplication). Also consider excluding unexported docs-input aggregates from the published src/ tree, after validating the packed file list and IDE/source-map expectations. Neither is browser-JS weight.
  • Destination handling: reuse the existing resolveShopifyDevPath helper rather than choosing ~/world/trees/root/src just because it exists. Prefer explicit synchronization separate from generation so an alternate worktree or missing shopify-dev zone does not receive unintended writes.
  • Shared docs fix: the committed Checkout/customer-account artifacts still contain the same native-member expansion (about 9.8/10.8 MB, with 60/68 declarations exposing native sentinels). A collector-level option to exclude inherited native-DOM members by declaration origin, while preserving authored members, would be more durable than independent regexes in each surface. Keep the output invariant and test it. These are follow-ups, not a request to expand this PR to all surfaces.

Validation: isolated docs generation and positive/negative guard probes, targeted type probes, and inspection of the green CI checks. I did not rerun the full build/test suite or the wrapper that automatically copies into World.

*/
error: AggregateError<T>;
error: AggregateError$1<T>;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from Kyle's agent review

Docs regression: can we restore a resolvable AggregateError docs entry before publishing this output? The base payload documented AggregateError.errors; all three head payloads now reference AggregateError$1<T> here, but contain neither an AggregateError nor an AggregateError$1 entry. This affects the FunctionSettings error documentation, so this particular removal is not just internal scaffolding cleanup.

The public-doc contract in the Polaris producer matches the unsuffixed name and misses the bundler-renamed declaration. Please normalize the alias/references and public-doc metadata consistently upstream, regenerate, and add a small reference-completeness fixture. The runtime API itself has not changed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from Kyle's agent review

Fixed in 4f01817. The regenerated aggregate and all three docs payloads now define public AggregateError.errors and AggregateErrorEvent.error references AggregateError<T>. The producer regression test is in the linked Polaris follow-up: https://meteorite.shopify.io/repos/shop/world/pulls/2062914

appHomeGeneratedDocsDataV2Path,
]);

await assertNoDomSurfaceLeak([

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from Kyle's agent review

Can we make temporary-file cleanup unconditional now that this intentionally throws on invalid output? fs.rm(tempComponentDefs) only runs at the end of the successful path. A rejected payload leaves src/surfaces/admin/components.ts behind; that file matches the package's TypeScript inputs and is under its published src/ tree. A scratch reproduction shows the next type-check fails on duplicate declarations/missing modules.

Move removal into finally and have copyGeneratedToShopifyDev return instead of calling process.exit() when no destination exists, since that also bypasses cleanup. Set process.exitCode at the entrypoint on failure. The lifecycle issue predates this PR, but this new deliberate failure path exposes it. Staging output before replacing the existing generated directory would also avoid partially written results.

'@shopify/ui-extensions': minor
---

Regenerate the Admin component definitions. Adds the missing `AppNav`, `EmptyState`, `Number`, `PressButton`, and `ScrollBox` types, plus `ProgressProps` in `shared.d.ts`. `Section` now accepts an `Icon` in its `graphic` slot and a `Tooltip` among its accessories.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from Kyle's agent review

Can we distinguish restored aggregate/docs coverage from newly available extension components here? AppNav.d.ts, EmptyState.d.ts, and Number.d.ts already existed at the base revision. PressButton and ScrollBox are restored only in the aggregate: this PR adds no public per-component export files or supported target-set entries for them. The current wording suggests consumers can start importing/using them.

The important consumer fix worth highlighting is that the missing shared ProgressProps definition now restores useful typing for Progress properties. I'd describe that fix, the Section/field documentation changes, and regeneration from Polaris 2.26.0 rather than advertising these five as new public component types. Keeping the minor changeset is appropriate.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from Kyle's agent review

Fixed in 4f01817. The changeset now calls out the repaired ProgressProps typing and Admin docs regeneration, without advertising aggregate-only PressButton or ScrollBox definitions as publicly usable components.

});
await replaceFileContent({
filePaths: tempComponentDefs,
searchValue: /declare const (\w+): \{[^{}]*\bHTMLElement\b[^{}]*\};/g,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from Kyle's agent review

\w+ does not include $, so the object-literal form stops matching if Rollup emits a normal deconflicted name such as BaseClass$2. The previous aggregate already used a $-suffixed native-base identifier. The sentinel correctly fails closed, but an ordinary naming variation would unnecessarily break docs generation.

Can we accept emitted JavaScript identifiers here (at minimum [\w$]+) and add fixtures for both historical forms, $ suffixes, whitespace variations, and an unrelated object that merely contains an HTMLElement-typed field? The last case also guards against over-neutralizing with the broad [^{}]*HTMLElement[^{}]* match. Extracting the rewrite and leak detection into importable pure helpers would let us test the negative case without running the whole script.


const assertNoDomSurfaceLeak = async (filePaths) => {
for (const filePath of filePaths) {
const docsData = JSON.parse(await fs.readFile(filePath, 'utf8'));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from Kyle's agent review

Measured docs-size opportunity: there are still 766 isPrivate: true member records originating in component declarations, including repeated rendering internals and TypeScript symbol IDs. A scoped pruning probe reduces each JSON from 1,683,007 to 1,363,054 bytes (19% raw; 171,881 → 152,806 bytes with gzip). This does not remove public inherited props, raw declaration strings, or dependent types. Please confirm the docs renderer does not need those private records before landing the pruning.

Rather than adding another file pass, could we make normalization/validation/pruning one pure in-memory pipeline? Optionality already parses and rewrites these files, and this function opens/parses them again. We also run the same TypeScript harvester twice over the same input even though all three current outputs are byte-identical. Generate once, then derive the destination files with any target-specific URL transformations preserved. This is docs/build overhead, not runtime bundle weight.

* @default 'start'
icon?:
| (
| 'adjust'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from Kyle's agent review

Polaris producer follow-up: can we preserve a reusable IconType alias instead of expanding the full icon-name union at each use? The 563-name union appears seven times in this aggregate, including unreferenced alias declarations. Roughly 70 KB of the aggregate's 150 KB increase comes from this duplication; the restored components account for only about 13 KB. The same expansion also adds weight to the docs metadata.

Shared declarations already had this emit pattern at the base revision, so this is pre-existing producer behavior newly visible in the regenerated aggregate, not a new runtime cost. Fix the declaration output upstream and regenerate rather than manually rewriting the union here.


/* eslint-disable @typescript-eslint/ban-types */
/* eslint-disable @typescript-eslint/no-namespace */
/* eslint-disable @typescript-eslint/member-ordering */

// eslint-disable-next-line @typescript-eslint/triple-slash-reference, spaced-comment
/// <reference lib="DOM" />
import * as preact$1 from 'preact';
import {ReactNode, RefAttributes} from 'react';
import * as _shopify_admin_web_component_foundations from '@shopify/admin-web-component-foundations';

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from Kyle's agent review

Pre-existing public-types issue, newly visible in this aggregate: this namespace import escapes the generator even though the public SDK does not depend on the restricted @shopify/admin-web-component-foundations package. The same import already exists at the base revision in the publicly exported components/Table.d.ts:18 and components/Clickable.d.ts:23; consumers checking declarations with skipLibCheck: false get TS2307. The aggregate itself is not a normal public entrypoint, so this is not a newly introduced consumer blocker in this PR.

The producer's import extraction handles named imports but misses namespace imports synthesized for inferred types. Please route namespace imports/qualified references through the same inlining and symbol-deconfliction process, and add an assertion that restricted-package imports cannot survive public emission. This belongs in a Foundations follow-up; adding the private runtime package as a dependency of the public SDK would be the wrong fix.


declare const internals$2: unique symbol;
declare class BaseClass$2 extends PreactCustomElement {
declare class BaseClass extends PolarisCustomElement {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from Kyle's agent review

Polaris producer follow-up: this class collides with declare const BaseClass at line 7036, the native HTMLElement constructor base emitted by the other bundler. TypeScript reports TS2451 at both declarations. After the docs rewrite makes the first declaration any, ColorPicker resolves against that declaration and loses eight inherited member records. Those records are all private today, so no public component props regress, but the generated input is order-dependent.

Can the declaration merge deconflict symbols across its independently emitted halves and reject duplicate non-mergeable top-level declarations? The native-DOM sentinel cannot detect authored inheritance silently disappearing. Renaming one local base can mitigate this instance, but a producer invariant is the durable fix.

@kyledurand
kyledurand merged commit 209e7a3 into 2026-10-rc Sep 14, 2026
6 checks passed
@kyledurand
kyledurand deleted the cx-regenerate-admin-ui-docs branch September 14, 2026 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants