diff --git a/agent-docs/lit-muscle-memory-gotchas.md b/agent-docs/lit-muscle-memory-gotchas.md index 80f271677..6fe882d5c 100644 --- a/agent-docs/lit-muscle-memory-gotchas.md +++ b/agent-docs/lit-muscle-memory-gotchas.md @@ -64,6 +64,66 @@ Practical consequences for agents writing webjs code. (optimistic UI, in-flight indicators tied to client state, keyboard shortcuts). +## Use lit idioms, not vanilla DOM (the whole point of lit-style components) + +webjs components are lit-shaped on purpose: the value is the declarative +DX (typed reactive props, signals, `html` templates, declarative +bindings), not raw DOM scripting. Reaching for vanilla web-component +muscle memory (`this.getAttribute`, `this.setAttribute`, `this.classList`, +`this.addEventListener`, `this.innerHTML`, `document.createElement`, +manual `observedAttributes` / `attributeChangedCallback`, manual +`customElements.define`) inside a component is the anti-pattern. Use the +lit form unless the vanilla API is genuinely unavoidable. + +| Vanilla muscle memory | Lit-style webjs form | +|---|---| +| `this.getAttribute('x')` / `this.hasAttribute('x')` for own config | a reactive prop: `static properties = { x: {...} }` + `declare x`, read `this.x` (the prop rides the `x` attribute) | +| `this.setAttribute('x', v)` / `removeAttribute` to reflect own state | a reactive prop with `reflect: true`, or for non-attribute state a `signal` | +| `state: true` reactive prop for internal state | a `signal` (instance signal in the constructor, or module-scope) | +| `this.classList.add/toggle(...)` on self | a `class=${...}` binding in `render()` | +| `this.innerHTML = ...` / `appendChild` / `document.createElement` | return the markup from `render()` as `` html`...` `` | +| `this.addEventListener('click', ...)` on own/child elements | a `@click=${...}` binding in the template | +| `this.querySelector(...)` to reach own rendered DOM | the `ref()` directive + `createRef()`, or read a `
` with `new FormData(form)` | +| manual `observedAttributes` + `attributeChangedCallback` | `static properties` (the framework derives both) | +| manual `customElements.define('x', C)` | `C.register('x')` | + +Emitting an event with `this.dispatchEvent(new CustomEvent(...))` is the +correct lit form, not a vanilla smell. Reading form values with +`new FormData(e.currentTarget)` inside a `@submit` handler is also fine. + +**When vanilla DOM is genuinely needed (these stay):** + +- **Ancestor lookup in a compound component.** `this.closest('ui-tabs')` + to read a parent's state. There is no declarative lit equivalent. +- **Slotted / projected content.** `this.querySelector(...)` reaching a + ``-projected child or a sibling sub-component the template does + not own. `ref()` only binds elements this component's own `render()` + creates, so it cannot reach slotted content. +- **Host attributes in light DOM.** A light-DOM `render()` template + cannot bind attributes or listeners on the host element itself, so a + component that must style or listen on its own host writes + `this.dataset.* =` / `this.className =` / `this.addEventListener` on + `this` in a lifecycle hook. Shadow-DOM components avoid this. +- **Global listeners.** `document` / `window` `addEventListener` for + click-away, global keys, resize, or reposition. +- **Reading another element's attribute.** `contentHost.getAttribute('side')` + reads a different element's config, not `this`. +- **Browser-only globals.** `localStorage`, `matchMedia`, `navigator`, + `document.documentElement` mutations (a theme toggle setting ``), + clipboard. These belong in `connectedCallback` or an event handler. +- **Imperative focus.** `el.focus()` has no declarative form. + +The rule of thumb: if a reactive prop, a signal, an `html` binding, or +`ref()` expresses it, use that. Reach for vanilla DOM only for the cases +above, where the platform offers nothing declarative. + +This is a **convention, not a lint rule.** `webjs check` is reserved for +general correctness (SSR safety, server-only imports, erasable TS, and +so on), not for policing every vanilla call, which would be noisy and +poor DX. Use your judgment: prefer the lit form by default, and when a +vanilla API genuinely has no declarative equivalent (the cases above), +just use it. + ## The SSR contract: the pre-render lifecycle plus `render()` By design, the webjs SSR pipeline constructs the instance, applies diff --git a/examples/blog/modules/chat/components/chat-box.ts b/examples/blog/modules/chat/components/chat-box.ts index 508783883..dc5e69a24 100644 --- a/examples/blog/modules/chat/components/chat-box.ts +++ b/examples/blog/modules/chat/components/chat-box.ts @@ -54,11 +54,10 @@ export class ChatBox extends WebComponent { onSubmit(e: SubmitEvent) { e.preventDefault(); const form = e.currentTarget as HTMLFormElement; - const input = form.querySelector('input') as HTMLInputElement; - const text = input.value.trim(); + const text = String(new FormData(form).get('message') ?? '').trim(); if (!text || !this._conn) return; this._conn.send({ text }); - input.value = ''; + form.reset(); } render() { @@ -99,7 +98,7 @@ export class ChatBox extends WebComponent { : html`

${l.text}

`)} this.onSubmit(e)}> - diff --git a/examples/blog/modules/comments/components/comments-thread.ts b/examples/blog/modules/comments/components/comments-thread.ts index 3b3549ad8..00a93a9d6 100644 --- a/examples/blog/modules/comments/components/comments-thread.ts +++ b/examples/blog/modules/comments/components/comments-thread.ts @@ -21,6 +21,7 @@ export class CommentsThread extends WebComponent { busy = signal(false); error = signal(null); _conn: ReturnType | null = null; + _seeded = false; constructor() { super(); @@ -29,9 +30,24 @@ export class CommentsThread extends WebComponent { this.signedIn = false; } + willUpdate(changed: Map) { + // Seed the live comment list from the server-provided `initial` prop the + // first time it is applied. willUpdate runs at SSR (and on the client's + // first render) AFTER props are applied, so the server-rendered first + // paint shows the real comments instead of the empty-state placeholder, + // rather than waiting for connectedCallback to fill it in on hydration. + // The `_seeded` guard means a later re-render never clobbers comments + // added live over the WebSocket or by a POST. + if (!this._seeded && changed.has('initial')) { + this._seeded = true; + if (Array.isArray(this.initial) && this.initial.length) { + this.comments.set(this.initial); + } + } + } + connectedCallback() { super.connectedCallback(); - this.comments.set(Array.isArray(this.initial) ? this.initial : []); this._conn = connectWS(`/api/comments/${this.postId}`, { onMessage: (msg: CommentFormatted) => { const cur = this.comments.get(); @@ -45,8 +61,7 @@ export class CommentsThread extends WebComponent { async onSubmit(e: SubmitEvent) { e.preventDefault(); const form = e.currentTarget as HTMLFormElement; - const input = form.querySelector('input') as HTMLInputElement; - const body = input.value.trim(); + const body = String(new FormData(form).get('body') ?? '').trim(); if (!body) return; this.busy.set(true); this.error.set(null); @@ -66,7 +81,7 @@ export class CommentsThread extends WebComponent { this.comments.set([...cur, created]); } this.busy.set(false); - input.value = ''; + form.reset(); } catch (err) { const msg = err instanceof Error ? err.message : String(err); this.busy.set(false); @@ -94,7 +109,7 @@ export class CommentsThread extends WebComponent { ${this.signedIn ? html` this.onSubmit(e)}> - diff --git a/examples/blog/test/comments/comments-ssr.test.ts b/examples/blog/test/comments/comments-ssr.test.ts new file mode 100644 index 000000000..0609f9fb7 --- /dev/null +++ b/examples/blog/test/comments/comments-ssr.test.ts @@ -0,0 +1,50 @@ +/** + * SSR regression for the comments thread first paint. + * + * The server-fetched `initial` comments are passed to as a + * prop. They must appear in the SSR'd HTML (the first paint), not pop in after + * hydration. The component seeds its live `comments` signal from `initial` in + * willUpdate (which runs at SSR as of the framework's pre-render lifecycle), so + * renderToString shows the real list. The counterfactual: with no initial + * comments, the empty-state placeholder is what renders. + * + * Run: node --test test/comments/comments-ssr.test.ts + */ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; + +import { html } from '@webjsdev/core'; +import { renderToString } from '@webjsdev/core/server'; +import '../../modules/comments/components/comments-thread.ts'; + +const SAMPLE = [ + { id: 'c1', authorName: 'Ada', createdAt: new Date('2020-01-01').toISOString(), body: 'first-ssr-comment' }, + { id: 'c2', authorName: 'Linus', createdAt: new Date('2020-01-02').toISOString(), body: 'second-ssr-comment' }, +]; + +// Assert on rendered CARD markup, not a body substring: the body text also +// appears inside the `initial="..."` attribute on the tag, so a bare substring +// match would false-pass even if the empty state rendered. The author name in +// a inside the card list is only present when a comment card rendered. +function rendersCards(out: string): boolean { + return /]*>[\s\S]*]*>Ada<\/strong>[\s\S]*]*>Linus<\/strong>/.test(out) + && !/No comments yet/.test(out); +} + +test('SSR renders the initial comments via the property form (not the empty state)', async () => { + const out = await renderToString(html``); + assert.ok(rendersCards(out), `property form must render comment cards, got:\n${out}`); +}); + +test('SSR renders the initial comments via the attribute form the post page uses', async () => { + // The post page renders `initial=${JSON.stringify(comments)}` (a string + // attribute), so this is the path that actually ships. It exercises both the + // willUpdate seed AND the Object-attribute entity decoding in the SSR walker. + const out = await renderToString(html``); + assert.ok(rendersCards(out), `attribute form must render comment cards, got:\n${out}`); +}); + +test('COUNTERFACTUAL: with no initial comments, the empty-state placeholder renders', async () => { + const out = await renderToString(html``); + assert.match(out, /No comments yet/, 'empty-state shown when there are no comments'); +}); diff --git a/packages/core/src/render-server.js b/packages/core/src/render-server.js index ce57afc4b..69120cc8f 100644 --- a/packages/core/src/render-server.js +++ b/packages/core/src/render-server.js @@ -854,7 +854,12 @@ function applyAttrsToInstance(instance, attrs, Cls) { if (def.type === Number) instance[propName] = Number(raw); else if (def.type === Boolean) instance[propName] = raw !== 'false'; else if (def.type === Object || def.type === Array) { - try { instance[propName] = JSON.parse(raw); } catch { instance[propName] = raw; } + // `raw` is the entity-encoded attribute text (parseAttrs returns the + // literal characters between the quotes), so decode the HTML entities + // before JSON.parse. A JSON attribute carries `"` for every `"`; + // parsing it raw throws and would silently fall back to the string, + // leaving an Object/Array prop holding a string at SSR. + try { instance[propName] = JSON.parse(unescapeAttr(raw)); } catch { instance[propName] = raw; } } else instance[propName] = raw; } } diff --git a/packages/core/test/rendering/ssr-lifecycle.test.js b/packages/core/test/rendering/ssr-lifecycle.test.js index b7653f4ac..bb51f6449 100644 --- a/packages/core/test/rendering/ssr-lifecycle.test.js +++ b/packages/core/test/rendering/ssr-lifecycle.test.js @@ -183,6 +183,30 @@ test('the server element shim mirrors lit: attributes getter, toggleAttribute, d assert.equal(secondAttachThrew, true, 'a second attachInternals throws, matching the browser and lit'); }); +test('an Object/Array attribute carrying JSON is entity-decoded before parse at SSR', async () => { + // A JSON value in an attribute is escaped to `"` by the renderer. The + // SSR walker must decode those entities before JSON.parse, or the prop ends + // up holding the raw string and render() sees the wrong type. Regression for + // applyAttrsToInstance. + let seenType = 'unset'; + class JsonAttr extends WebComponent { + static properties = { data: { type: Object } }; + constructor() { super(); this.data = null; } + render() { + seenType = Array.isArray(this.data) ? 'array' : typeof this.data; + const first = Array.isArray(this.data) ? this.data[0]?.label : undefined; + return html`

${first ?? 'none'}

`; + } + } + JsonAttr.register('ssr-json-attr'); + + const payload = [{ label: 'has "quotes" & ' }]; + const out = await renderToString(html``); + assert.equal(seenType, 'array', 'the Object attribute parsed to an array, not a string'); + // In text content, `&` and `<`/`>` are escaped but quotes stay literal. + assert.match(out, /

has "quotes" & <ammp><\/p>/, 'the decoded value rendered correctly'); +}); + test('COUNTERFACTUAL: without the willUpdate pass, the derived value would be the constructor placeholder', async () => { // Mirrors the first test but proves the assertion would FAIL if willUpdate // did not run: a subclass that deliberately renders the raw constructor diff --git a/packages/ui/packages/registry/components/hover-card.ts b/packages/ui/packages/registry/components/hover-card.ts index dbe3d41de..98145a2cf 100644 --- a/packages/ui/packages/registry/components/hover-card.ts +++ b/packages/ui/packages/registry/components/hover-card.ts @@ -55,8 +55,14 @@ export const hoverCardContentClass = (): string => export class UiHoverCard extends WebComponent { static properties = { open: { type: Boolean, reflect: true }, + openDelay: { type: Number }, + closeDelay: { type: Number }, }; declare open: boolean; + // `openDelay` / `closeDelay` ride the `open-delay` / `close-delay` + // attributes (shadcn parity), read as typed props. + declare openDelay: number; + declare closeDelay: number; _showTimer: number | undefined; _hideTimer: number | undefined; @@ -64,6 +70,8 @@ export class UiHoverCard extends WebComponent { constructor() { super(); this.open = false; + this.openDelay = 700; + this.closeDelay = 300; } // Back-compat getter. @@ -71,14 +79,12 @@ export class UiHoverCard extends WebComponent { show(): void { clearTimeout(this._hideTimer); - const delay = Number(this.getAttribute('open-delay') ?? 700); - this._showTimer = window.setTimeout(() => { this.open = true; }, delay); + this._showTimer = window.setTimeout(() => { this.open = true; }, this.openDelay); } hide(): void { clearTimeout(this._showTimer); - const delay = Number(this.getAttribute('close-delay') ?? 300); - this._hideTimer = window.setTimeout(() => { this.open = false; }, delay); + this._hideTimer = window.setTimeout(() => { this.open = false; }, this.closeDelay); } render() { diff --git a/packages/ui/packages/registry/components/tooltip.ts b/packages/ui/packages/registry/components/tooltip.ts index 1d27c2cf7..7312598bc 100644 --- a/packages/ui/packages/registry/components/tooltip.ts +++ b/packages/ui/packages/registry/components/tooltip.ts @@ -64,8 +64,14 @@ let lastTooltipHideAt = 0; export class UiTooltip extends WebComponent { static properties = { open: { type: Boolean, reflect: true }, + delayDuration: { type: Number }, + skipDelayDuration: { type: Number }, }; declare open: boolean; + // `delayDuration` / `skipDelayDuration` ride the `delay-duration` / + // `skip-delay-duration` attributes (shadcn parity), read as typed props. + declare delayDuration: number; + declare skipDelayDuration: number; _showTimer: number | undefined; _hideTimer: number | undefined; @@ -73,6 +79,8 @@ export class UiTooltip extends WebComponent { constructor() { super(); this.open = false; + this.delayDuration = 700; + this.skipDelayDuration = 300; } // Back-compat getter for tests + external code that read `el.isOpen` @@ -82,8 +90,8 @@ export class UiTooltip extends WebComponent { show(): void { clearTimeout(this._showTimer); clearTimeout(this._hideTimer); - const delay = Number(this.getAttribute('delay-duration') ?? 700); - const skipDelay = Number(this.getAttribute('skip-delay-duration') ?? 300); + const delay = this.delayDuration; + const skipDelay = this.skipDelayDuration; const sinceLastHide = Date.now() - lastTooltipHideAt; if (lastTooltipHideAt > 0 && sinceLastHide < skipDelay) { this.open = true;