From 8171d6358a659131d921a4df50d8c718468fb634 Mon Sep 17 00:00:00 2001 From: t Date: Tue, 2 Jun 2026 19:50:59 +0530 Subject: [PATCH 1/9] fix: seed comments-thread from initial in willUpdate so SSR shows them The thread rendered from a comments signal seeded only in connectedCallback, so the server-fetched initial comments were absent from the SSR first paint and popped in on hydration (a progressive- enhancement violation, visible empty-state flash on the post page). Seed the signal from the initial prop in willUpdate, which now runs at SSR after props are applied, so the first paint shows the real list. A _seeded guard keeps live WebSocket / POST additions from being clobbered by a later re-render. --- .../comments/components/comments-thread.ts | 18 +++++++++- .../blog/test/comments/comments-ssr.test.ts | 35 +++++++++++++++++++ 2 files changed, 52 insertions(+), 1 deletion(-) create mode 100644 examples/blog/test/comments/comments-ssr.test.ts diff --git a/examples/blog/modules/comments/components/comments-thread.ts b/examples/blog/modules/comments/components/comments-thread.ts index 3b3549ad8..3446fc638 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(); 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..ede385518 --- /dev/null +++ b/examples/blog/test/comments/comments-ssr.test.ts @@ -0,0 +1,35 @@ +/** + * 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' }, +]; + +test('SSR renders the server-provided initial comments (not the empty state)', async () => { + const out = await renderToString(html``); + assert.match(out, /first-ssr-comment/, 'first initial comment is in the SSR HTML'); + assert.match(out, /second-ssr-comment/, 'second initial comment is in the SSR HTML'); + assert.doesNotMatch(out, /No comments yet/, 'the empty-state placeholder is not shown when comments exist'); +}); + +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'); +}); From da6dbff8b93e38773a209244bfed2175353d1674 Mon Sep 17 00:00:00 2001 From: t Date: Tue, 2 Jun 2026 20:03:30 +0530 Subject: [PATCH 2/9] fix: decode HTML entities before JSON.parse for Object/Array attrs at SSR applyAttrsToInstance parsed the raw entity-encoded attribute text, so a JSON value in an attribute (every quote escaped to ") threw and silently fell back to the raw string, leaving an Object/Array prop holding a string at SSR. seedServerAttrs and consumePropAttrs already unescape; applyAttrsToInstance now does too. This is what made the attribute form of a rich prop (e.g. comments-thread initial=JSON) render the empty state server-side. --- packages/core/src/render-server.js | 7 +++++- .../core/test/rendering/ssr-lifecycle.test.js | 24 +++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) 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 From ba43825e61b5824393c161688c63670035cd14a0 Mon Sep 17 00:00:00 2001 From: t Date: Tue, 2 Jun 2026 20:03:40 +0530 Subject: [PATCH 3/9] test: cover comments-thread SSR via the attribute form the page uses Strengthen the SSR regression to assert on rendered comment-card markup (author names in the list), not a body substring that also appears in the initial attribute, and add the attribute-form case (the path the post page actually ships) alongside the property form. --- .../blog/test/comments/comments-ssr.test.ts | 23 +++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/examples/blog/test/comments/comments-ssr.test.ts b/examples/blog/test/comments/comments-ssr.test.ts index ede385518..0609f9fb7 100644 --- a/examples/blog/test/comments/comments-ssr.test.ts +++ b/examples/blog/test/comments/comments-ssr.test.ts @@ -22,11 +22,26 @@ const SAMPLE = [ { id: 'c2', authorName: 'Linus', createdAt: new Date('2020-01-02').toISOString(), body: 'second-ssr-comment' }, ]; -test('SSR renders the server-provided initial comments (not the empty state)', async () => { +// 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.match(out, /first-ssr-comment/, 'first initial comment is in the SSR HTML'); - assert.match(out, /second-ssr-comment/, 'second initial comment is in the SSR HTML'); - assert.doesNotMatch(out, /No comments yet/, 'the empty-state placeholder is not shown when comments exist'); + 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 () => { From 9ab08a18cd8ad2cfd24b958cf51993f68b91f42f Mon Sep 17 00:00:00 2001 From: t Date: Tue, 2 Jun 2026 20:11:18 +0530 Subject: [PATCH 4/9] refactor: read tooltip/hover-card delay config via reactive props Replace this.getAttribute('delay-duration' / 'open-delay' / ...) with typed reactive properties (delayDuration, skipDelayDuration, openDelay, closeDelay) that ride the same hyphenated attributes (shadcn parity). The lit-idiomatic property API is the better DX and the webjs-preferred way to read attribute-backed config; raw getAttribute is reserved for when a reactive prop will not do. No behavior or attribute-API change. --- .../ui/packages/registry/components/hover-card.ts | 14 ++++++++++---- .../ui/packages/registry/components/tooltip.ts | 12 ++++++++++-- 2 files changed, 20 insertions(+), 6 deletions(-) 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; From 0dd7127a27c004ffc1d118254330603cd029593a Mon Sep 17 00:00:00 2001 From: t Date: Tue, 2 Jun 2026 20:43:23 +0530 Subject: [PATCH 5/9] refactor: read chat/comments form input via FormData, not querySelector Replace form.querySelector('input') + input.value='' with new FormData(form).get(name) + form.reset(), the same form-read idiom auth-forms already uses. Removes the imperative own-DOM query in favour of the standard form API. Inputs gain name= so FormData can read them. --- examples/blog/modules/chat/components/chat-box.ts | 7 +++---- .../blog/modules/comments/components/comments-thread.ts | 7 +++---- 2 files changed, 6 insertions(+), 8 deletions(-) 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 3446fc638..00a93a9d6 100644 --- a/examples/blog/modules/comments/components/comments-thread.ts +++ b/examples/blog/modules/comments/components/comments-thread.ts @@ -61,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); @@ -82,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); @@ -110,7 +109,7 @@ export class CommentsThread extends WebComponent { ${this.signedIn ? html` this.onSubmit(e)}> -
From ec6e864a0ef254ce7ccfd3a23d4e93d45486b522 Mon Sep 17 00:00:00 2001 From: t Date: Tue, 2 Jun 2026 20:50:10 +0530 Subject: [PATCH 6/9] feat: document + enforce lit-style components over vanilla DOM Add a 'use lit idioms, not vanilla DOM' convention to the lit-muscle-memory gotchas (vanilla->lit table + the genuinely-needed exceptions: closest, slotted querySelector, host writes in light DOM, global listeners, browser-only globals). Add a webjs check rule, prefer-reactive-prop-over-getattribute, flagging this.getAttribute('x') for own config in a WebComponent (allowlisting class/style/aria-*/data-* and dynamic names), steering to a reactive property. Reserves getAttribute for reading another element's attribute. --- agent-docs/lit-muscle-memory-gotchas.md | 55 +++++++++++ packages/server/src/check.js | 68 ++++++++++++++ ...er-reactive-prop-over-getattribute.test.js | 94 +++++++++++++++++++ 3 files changed, 217 insertions(+) create mode 100644 packages/server/test/check/prefer-reactive-prop-over-getattribute.test.js diff --git a/agent-docs/lit-muscle-memory-gotchas.md b/agent-docs/lit-muscle-memory-gotchas.md index 80f271677..f004e09da 100644 --- a/agent-docs/lit-muscle-memory-gotchas.md +++ b/agent-docs/lit-muscle-memory-gotchas.md @@ -64,6 +64,61 @@ 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. `webjs check`'s +`prefer-reactive-prop-over-getattribute` rule flags the most common slip +(reading own config via `this.getAttribute`). + ## The SSR contract: the pre-render lifecycle plus `render()` By design, the webjs SSR pipeline constructs the instance, applies diff --git a/packages/server/src/check.js b/packages/server/src/check.js index 54a08a9ed..601d04633 100644 --- a/packages/server/src/check.js +++ b/packages/server/src/check.js @@ -119,6 +119,11 @@ export const RULES = [ description: 'Flags genuinely browser-only APIs used in a WebComponent constructor, willUpdate, or render() method. The SSR pipeline instantiates the component, runs willUpdate plus controllers\' hostUpdate, reflects properties, and calls render() to produce HTML, on a server element shim that backs the attribute methods but has no real DOM. So a browser global (document, window, localStorage, sessionStorage, navigator, location, matchMedia, screen, history) or an unshimmed HTMLElement member on `this` (attachShadow, shadowRoot, classList, querySelector, querySelectorAll, getBoundingClientRect, focus, blur, scrollIntoView) touched there throws at SSR time (the isomorphic footgun). The attribute methods (getAttribute/setAttribute/hasAttribute/removeAttribute/toggleAttribute), the event methods (addEventListener/removeEventListener/dispatchEvent), and attachInternals are shim-backed and run server-side, so they are NOT flagged. The flagged APIs belong in connectedCallback() or a lifecycle hook (firstUpdated/updated), which SSR never calls; seed first-paint defaults in the constructor (or derive them in willUpdate) only from server-known inputs (attributes, props). Conservative: only the constructor, willUpdate, and render bodies are scanned, and only direct references, so helper indirection is not flagged (the runtime SSR error covers that case).', }, + { + name: 'prefer-reactive-prop-over-getattribute', + description: + 'webjs components are lit-shaped: read your own config through a reactive property (static properties + declare, read this.x), not by calling this.getAttribute, which is vanilla web-component muscle memory. Flags this.getAttribute(\'name\') with a literal attribute name inside a WebComponent class body. Standard attributes that are not modelled as typed props are allowlisted (class, style, id, is, slot, part, title, lang, dir, role, hidden, tabindex, name, type, value, and any aria-* / data-* name), as are dynamic names (this.getAttribute(variable)) and reads off another element (only this.getAttribute on `this` is flagged). The fix is a reactive property whose camelCase name rides the hyphenated attribute. Reserve getAttribute for reading a different element\'s attribute or a standard attribute with native semantics. The companion hasAttribute pattern is covered by the prose convention. See agent-docs/lit-muscle-memory-gotchas.md.', + }, ]; /** Set of all known rule names for fast lookup. */ @@ -465,6 +470,47 @@ function findBrowserMemberUses(code) { return out; } +// Standard attributes that are NOT modelled as typed reactive props, so +// reading them via getAttribute/hasAttribute is legitimate (not flagged). +const ALLOWED_ATTR_READS = new Set([ + 'class', 'style', 'id', 'is', 'slot', 'part', 'title', 'lang', 'dir', + 'role', 'hidden', 'tabindex', 'name', 'type', 'value', +]); + +/** + * Find `this.getAttribute('name')` calls with a literal, non-allowlisted + * attribute name in a (redacted) WebComponent class body. Dynamic names, + * allowlisted standard attributes, and aria-* / data-* are skipped. Reads off + * another element (e.g. `host.getAttribute`) are not matched because the + * pattern is anchored to `this.`. Only `getAttribute` is matched (the config + * read the lit prop API replaces); `hasAttribute` is covered by the prose + * convention in agent-docs/lit-muscle-memory-gotchas.md. + * + * @param {string} classBody + * @returns {{ method: string, attr: string }[]} + */ +/** `delay-duration` -> `delayDuration` (attribute name to reactive-prop name). */ +function attrToCamel(s) { + return s.replace(/-([a-z])/g, (_, c) => c.toUpperCase()); +} + +function findOwnAttributeReads(classBody) { + const out = []; + const seen = new Set(); + const re = /\bthis\.(getAttribute)\(\s*(['"])([^'"]+)\2/g; + let m; + while ((m = re.exec(classBody)) !== null) { + const method = m[1]; + const attr = m[3].toLowerCase(); + if (ALLOWED_ATTR_READS.has(attr) || attr.startsWith('aria-') || attr.startsWith('data-')) continue; + const key = `${method}:${attr}`; + if (seen.has(key)) continue; + seen.add(key); + out.push({ method, attr }); + } + return out; +} + /** * Scan a webjs app directory and report convention violations. * @@ -660,6 +706,28 @@ export async function checkConventions(appDir, opts) { } } + // --- Rule: prefer-reactive-prop-over-getattribute --- + // Lit-shaped components read their own config through a reactive property + // (this.x), not this.getAttribute('x') / this.hasAttribute('x') (vanilla + // muscle memory). Flags only literal, non-allowlisted attribute names on + // `this`; standard attributes (class/style/id/...), aria-*/data-*, dynamic + // names, and reads off another element are not flagged. + if (isRuleEnabled('prefer-reactive-prop-over-getattribute', overrides)) { + for (const { rel, scan } of files) { + if (!/class\s+\w+\s+extends\s+WebComponent/.test(scan)) continue; + for (const body of extractWebComponentClassBodies(scan)) { + for (const { method, attr } of findOwnAttributeReads(body)) { + violations.push({ + rule: 'prefer-reactive-prop-over-getattribute', + file: rel, + message: `\`this.${method}('${attr}')\` reads own config via a vanilla attribute call. Use a reactive property instead.`, + fix: `Declare \`${attrToCamel(attr)}\` in \`static properties\` with a \`declare ${attrToCamel(attr)}\` field (the prop rides the \`${attr}\` attribute), then read \`this.${attrToCamel(attr)}\`. Reserve getAttribute/hasAttribute for reading another element's attribute or a standard attribute with native semantics.`, + }); + } + } + } + } + // --- Rule: no-server-env-in-components --- // Catches `process.env.X` reads in component files where X is not a // WEBJS_PUBLIC_* var and not NODE_ENV. The SSR shim only exposes those diff --git a/packages/server/test/check/prefer-reactive-prop-over-getattribute.test.js b/packages/server/test/check/prefer-reactive-prop-over-getattribute.test.js new file mode 100644 index 000000000..e0587515d --- /dev/null +++ b/packages/server/test/check/prefer-reactive-prop-over-getattribute.test.js @@ -0,0 +1,94 @@ +/** + * Tests for the prefer-reactive-prop-over-getattribute rule. webjs components + * are lit-shaped: read own config through a reactive property, not via the + * vanilla this.getAttribute('name'). The rule flags literal, non-allowlisted + * own-attribute reads and steers to a reactive prop. + */ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { mkdtemp, mkdir, writeFile, rm } from 'node:fs/promises'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; + +import { checkConventions } from '../../src/check.js'; + +async function appWith(rel, contents) { + const dir = await mkdtemp(join(tmpdir(), 'webjs-rp-')); + const filePath = join(dir, rel); + await mkdir(filePath.slice(0, filePath.lastIndexOf('/')), { recursive: true }); + await writeFile(filePath, contents); + return dir; +} +const find = (vs, file) => vs.filter((v) => v.rule === 'prefer-reactive-prop-over-getattribute' && v.file.includes(file)); + +test('flags this.getAttribute for own config in a component', async () => { + const dir = await appWith('components/tip.ts', ` +import { WebComponent, html } from '@webjsdev/core'; +export class Tip extends WebComponent { + show() { + const d = Number(this.getAttribute('delay-duration') ?? 700); + return d; + } + render() { return html\`

\`; } +} +Tip.register('tip-el'); +`); + try { + const v = find(await checkConventions(dir), 'tip.ts'); + assert.ok(v.some((x) => x.message.includes("this.getAttribute('delay-duration')")), 'own-config getAttribute flagged'); + assert.ok(v.some((x) => x.fix.includes('delayDuration')), 'fix names the camelCase reactive prop'); + } finally { await rm(dir, { recursive: true, force: true }); } +}); + +test('does NOT flag allowlisted standard attributes (class) or aria-/data-', async () => { + const dir = await appWith('components/ok.ts', ` +import { WebComponent, html } from '@webjsdev/core'; +export class Ok extends WebComponent { + render() { + const c = this.getAttribute('class') ?? ''; + const a = this.getAttribute('aria-label'); + const d = this.getAttribute('data-x'); + return html\`

\${c}\${a}\${d}

\`; + } +} +Ok.register('ok-el'); +`); + try { + assert.equal(find(await checkConventions(dir), 'ok.ts').length, 0, 'class / aria-* / data-* must not be flagged'); + } finally { await rm(dir, { recursive: true, force: true }); } +}); + +test('does NOT flag a read off another element, or a dynamic name', async () => { + const dir = await appWith('components/other.ts', ` +import { WebComponent, html } from '@webjsdev/core'; +export class Other extends WebComponent { + _read(host, key) { + const a = host.getAttribute('side'); // another element + const b = this.getAttribute(key); // dynamic name + return [a, b]; + } + render() { return html\`

\`; } +} +Other.register('other-el'); +`); + try { + assert.equal(find(await checkConventions(dir), 'other.ts').length, 0, 'another element + dynamic name must not be flagged'); + } finally { await rm(dir, { recursive: true, force: true }); } +}); + +test('counterfactual control: a reactive-prop component is not flagged', async () => { + const dir = await appWith('components/clean.ts', ` +import { WebComponent, html } from '@webjsdev/core'; +export class Clean extends WebComponent { + static properties = { delayDuration: { type: Number } }; + declare delayDuration; + constructor() { super(); this.delayDuration = 700; } + show() { return this.delayDuration; } + render() { return html\`

\`; } +} +Clean.register('clean-el'); +`); + try { + assert.equal(find(await checkConventions(dir), 'clean.ts').length, 0, 'reading the reactive prop is the correct, unflagged form'); + } finally { await rm(dir, { recursive: true, force: true }); } +}); From 14117c165997539246bd9fea9427899b3c51cdea Mon Sep 17 00:00:00 2001 From: t Date: Tue, 2 Jun 2026 20:51:35 +0530 Subject: [PATCH 7/9] feat: add prefer-signal-over-state-prop check rule Enforce framework invariant 5 (signals for internal state, reactive properties only for attribute-backed values): flag a state: true reactive property in a WebComponent and steer to a signal. Companion to prefer-reactive-prop-over-getattribute; both keep components lit-shaped without leaning on vanilla / lit-internal-state patterns webjs replaces with signals and reactive props. --- packages/server/src/check.js | 25 +++++++ .../prefer-signal-over-state-prop.test.js | 67 +++++++++++++++++++ 2 files changed, 92 insertions(+) create mode 100644 packages/server/test/check/prefer-signal-over-state-prop.test.js diff --git a/packages/server/src/check.js b/packages/server/src/check.js index 601d04633..40dc5dbee 100644 --- a/packages/server/src/check.js +++ b/packages/server/src/check.js @@ -124,6 +124,11 @@ export const RULES = [ description: 'webjs components are lit-shaped: read your own config through a reactive property (static properties + declare, read this.x), not by calling this.getAttribute, which is vanilla web-component muscle memory. Flags this.getAttribute(\'name\') with a literal attribute name inside a WebComponent class body. Standard attributes that are not modelled as typed props are allowlisted (class, style, id, is, slot, part, title, lang, dir, role, hidden, tabindex, name, type, value, and any aria-* / data-* name), as are dynamic names (this.getAttribute(variable)) and reads off another element (only this.getAttribute on `this` is flagged). The fix is a reactive property whose camelCase name rides the hyphenated attribute. Reserve getAttribute for reading a different element\'s attribute or a standard attribute with native semantics. The companion hasAttribute pattern is covered by the prose convention. See agent-docs/lit-muscle-memory-gotchas.md.', }, + { + name: 'prefer-signal-over-state-prop', + description: + 'Flags a `state: true` reactive property in a WebComponent\'s static properties. webjs reserves reactive properties for values that ride an HTML attribute (or arrive via .prop SSR hydration); internal reactive state with no attribute is held in a signal instead (framework invariant 5). lit uses `state: true` for attribute-less internal state, but in webjs that should be a `signal` (an instance signal created in the constructor, or a module-scope signal for shared state), read via signal.get() inside render(). Replace the `state: true` declaration with a signal field.', + }, ]; /** Set of all known rule names for fast lookup. */ @@ -728,6 +733,26 @@ export async function checkConventions(appDir, opts) { } } + // --- Rule: prefer-signal-over-state-prop --- + // `state: true` is a reactive property with no attribute (lit's internal + // state). webjs holds attribute-less internal state in a signal (invariant + // 5), so flag `state: true` in a WebComponent's static properties. + if (isRuleEnabled('prefer-signal-over-state-prop', overrides)) { + for (const { rel, scan } of files) { + if (!/class\s+\w+\s+extends\s+WebComponent/.test(scan)) continue; + for (const body of extractWebComponentClassBodies(scan)) { + if (/\bstate\s*:\s*true\b/.test(body)) { + violations.push({ + rule: 'prefer-signal-over-state-prop', + file: rel, + message: `A \`state: true\` reactive property holds internal state with no attribute. webjs uses a signal for that.`, + fix: `Remove the \`state: true\` declaration and hold the value in a signal: create an instance signal in the constructor (or a module-scope signal for shared state) and read it via signal.get() inside render(). Reactive properties are reserved for values that ride an HTML attribute.`, + }); + } + } + } + } + // --- Rule: no-server-env-in-components --- // Catches `process.env.X` reads in component files where X is not a // WEBJS_PUBLIC_* var and not NODE_ENV. The SSR shim only exposes those diff --git a/packages/server/test/check/prefer-signal-over-state-prop.test.js b/packages/server/test/check/prefer-signal-over-state-prop.test.js new file mode 100644 index 000000000..c768e890d --- /dev/null +++ b/packages/server/test/check/prefer-signal-over-state-prop.test.js @@ -0,0 +1,67 @@ +/** + * Tests for prefer-signal-over-state-prop. webjs reserves reactive properties + * for attribute-backed values; internal reactive state with no attribute is a + * signal (invariant 5). A `state: true` reactive property is the lit idiom + * webjs replaces with a signal. + */ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { mkdtemp, mkdir, writeFile, rm } from 'node:fs/promises'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; + +import { checkConventions } from '../../src/check.js'; + +async function appWith(rel, contents) { + const dir = await mkdtemp(join(tmpdir(), 'webjs-ss-')); + const filePath = join(dir, rel); + await mkdir(filePath.slice(0, filePath.lastIndexOf('/')), { recursive: true }); + await writeFile(filePath, contents); + return dir; +} +const find = (vs, file) => vs.filter((v) => v.rule === 'prefer-signal-over-state-prop' && v.file.includes(file)); + +test('flags a state: true reactive property', async () => { + const dir = await appWith('components/s.ts', ` +import { WebComponent, html } from '@webjsdev/core'; +export class S extends WebComponent { + static properties = { open: { type: Boolean, state: true } }; + declare open; + render() { return html\`

\`; } +} +S.register('s-el'); +`); + try { + assert.equal(find(await checkConventions(dir), 's.ts').length, 1, 'state: true must be flagged'); + } finally { await rm(dir, { recursive: true, force: true }); } +}); + +test('counterfactual: a signal-backed component (no state: true) is not flagged', async () => { + const dir = await appWith('components/sig.ts', ` +import { WebComponent, html, signal } from '@webjsdev/core'; +export class Sig extends WebComponent { + open = signal(false); + render() { return html\`

\${this.open.get()}

\`; } +} +Sig.register('sig-el'); +`); + try { + assert.equal(find(await checkConventions(dir), 'sig.ts').length, 0, 'a signal is the correct, unflagged form'); + } finally { await rm(dir, { recursive: true, force: true }); } +}); + +test('does not flag an attribute-backed reactive prop (reflect, no state)', async () => { + const dir = await appWith('components/attr.ts', ` +import { WebComponent, html } from '@webjsdev/core'; +export class Attr extends WebComponent { + static properties = { open: { type: Boolean, reflect: true } }; + declare open; + constructor() { super(); this.open = false; } + render() { return html\`

\`; } +} +Attr.register('attr-el'); +`); + try { + assert.equal(find(await checkConventions(dir), 'attr.ts').length, 0, 'attribute-backed reactive props are correct'); + } finally { await rm(dir, { recursive: true, force: true }); } +}); From 85a2b4339b38332a4e24f926cea70eddf4bc7260 Mon Sep 17 00:00:00 2001 From: t Date: Tue, 2 Jun 2026 20:57:05 +0530 Subject: [PATCH 8/9] feat: per-file webjs-check-ignore escape hatch for check rules Add an inline opt-out: a file may suppress a rule with // webjs-check-ignore [,] (or * for all) anywhere in it. The package.json conventions switch disables a rule project-wide; this is the finer per-file escape hatch so a component that genuinely needs a flagged pattern (a vanilla DOM call with no lit equivalent) can keep it without turning the rule off everywhere. Document both escape levels in the lit-muscle-memory convention. --- agent-docs/lit-muscle-memory-gotchas.md | 18 ++++- packages/server/src/check.js | 27 ++++++++ .../server/test/check/inline-ignore.test.js | 68 +++++++++++++++++++ 3 files changed, 110 insertions(+), 3 deletions(-) create mode 100644 packages/server/test/check/inline-ignore.test.js diff --git a/agent-docs/lit-muscle-memory-gotchas.md b/agent-docs/lit-muscle-memory-gotchas.md index f004e09da..9bd20b3c7 100644 --- a/agent-docs/lit-muscle-memory-gotchas.md +++ b/agent-docs/lit-muscle-memory-gotchas.md @@ -115,9 +115,21 @@ correct lit form, not a vanilla smell. Reading form values with 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. `webjs check`'s -`prefer-reactive-prop-over-getattribute` rule flags the most common slip -(reading own config via `this.getAttribute`). +above, where the platform offers nothing declarative. Two `webjs check` +rules enforce the highest-confidence slips: +`prefer-reactive-prop-over-getattribute` (reading own config via +`this.getAttribute`) and `prefer-signal-over-state-prop` (a `state: true` +reactive property that should be a signal). + +**You can still use a flagged pattern when it is genuinely needed.** Both +the project-level switch and a per-file escape hatch exist: + +- Turn a rule off across the project in `package.json` under + `"webjs": { "conventions": { "prefer-reactive-prop-over-getattribute": false } }`. +- Suppress it for ONE file with a comment, keeping the rule on everywhere + else: `// webjs-check-ignore prefer-reactive-prop-over-getattribute ` + (use `*` to suppress every rule for that file). This is the intended way + to keep a deliberate vanilla call that has no clean lit equivalent. ## The SSR contract: the pre-render lifecycle plus `render()` diff --git a/packages/server/src/check.js b/packages/server/src/check.js index 40dc5dbee..14400e361 100644 --- a/packages/server/src/check.js +++ b/packages/server/src/check.js @@ -1197,6 +1197,33 @@ export async function checkConventions(appDir, opts) { } } + // Inline opt-out (the per-occurrence escape hatch). A file may suppress a + // rule with a `// webjs-check-ignore [,]` comment (or `* ` for + // all rules) anywhere in the file. The package.json `webjs.conventions` + // switch turns a rule off across the whole project; this is the finer, + // per-file escape hatch so a component that genuinely needs a flagged + // pattern (a vanilla DOM call with no lit equivalent, a deliberate browser + // global) can keep it without disabling the rule everywhere. Authors should + // add a short reason after the rule name. + const ignoresByRel = new Map(); + for (const { rel, content } of files) { + const ignored = new Set(); + const re = /webjs-check-ignore[ \t]+([^\n]+)/gi; + let m; + while ((m = re.exec(content)) !== null) { + for (const tok of m[1].split(/[\s,]+/)) { + if (tok === '*' || RULE_NAMES.has(tok)) ignored.add(tok); + } + } + if (ignored.size) ignoresByRel.set(rel, ignored); + } + if (ignoresByRel.size) { + return violations.filter((v) => { + const ig = ignoresByRel.get(v.file); + return !ig || !(ig.has(v.rule) || ig.has('*')); + }); + } + return violations; } diff --git a/packages/server/test/check/inline-ignore.test.js b/packages/server/test/check/inline-ignore.test.js new file mode 100644 index 000000000..2778d1f4b --- /dev/null +++ b/packages/server/test/check/inline-ignore.test.js @@ -0,0 +1,68 @@ +/** + * The per-file inline escape hatch: `// webjs-check-ignore ` suppresses + * that rule for the file, so a component that genuinely needs a flagged + * pattern can keep it without disabling the rule project-wide. `*` suppresses + * all rules for the file. + */ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { mkdtemp, mkdir, writeFile, rm } from 'node:fs/promises'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; + +import { checkConventions } from '../../src/check.js'; + +async function appWith(rel, contents) { + const dir = await mkdtemp(join(tmpdir(), 'webjs-ig-')); + const filePath = join(dir, rel); + await mkdir(filePath.slice(0, filePath.lastIndexOf('/')), { recursive: true }); + await writeFile(filePath, contents); + return dir; +} +const ofRule = (vs, rule, file) => vs.filter((v) => v.rule === rule && v.file.includes(file)); + +const FLAGGED = ` +import { WebComponent, html } from '@webjsdev/core'; +export class C extends WebComponent { + show() { return Number(this.getAttribute('delay-duration') ?? 700); } + render() { return html\`

\`; } +} +C.register('c-el'); +`; + +test('without the directive, the rule fires', async () => { + const dir = await appWith('components/c.ts', FLAGGED); + try { + assert.equal(ofRule(await checkConventions(dir), 'prefer-reactive-prop-over-getattribute', 'c.ts').length, 1); + } finally { await rm(dir, { recursive: true, force: true }); } +}); + +test('webjs-check-ignore suppresses that rule for the file', async () => { + const dir = await appWith('components/c.ts', + `// webjs-check-ignore prefer-reactive-prop-over-getattribute reading a legacy attr\n${FLAGGED}`); + try { + assert.equal(ofRule(await checkConventions(dir), 'prefer-reactive-prop-over-getattribute', 'c.ts').length, 0, + 'the inline directive suppresses the violation'); + } finally { await rm(dir, { recursive: true, force: true }); } +}); + +test('webjs-check-ignore * suppresses all rules for the file', async () => { + const dir = await appWith('components/c.ts', `// webjs-check-ignore *\n${FLAGGED}`); + try { + assert.equal(ofRule(await checkConventions(dir), 'prefer-reactive-prop-over-getattribute', 'c.ts').length, 0); + } finally { await rm(dir, { recursive: true, force: true }); } +}); + +test('the directive only affects the file it is in', async () => { + const dir = await appWith('components/c.ts', FLAGGED); + // a second flagged file without the directive still fires + const fp = join(dir, 'components/d.ts'); + await writeFile(fp, FLAGGED.replace(/\bC\b/g, 'D').replace('c-el', 'd-el')); + await writeFile(join(dir, 'components/c.ts'), + `// webjs-check-ignore prefer-reactive-prop-over-getattribute\n${FLAGGED}`); + try { + const vs = await checkConventions(dir); + assert.equal(ofRule(vs, 'prefer-reactive-prop-over-getattribute', 'c.ts').length, 0, 'c.ts suppressed'); + assert.equal(ofRule(vs, 'prefer-reactive-prop-over-getattribute', 'd.ts').length, 1, 'd.ts still fires'); + } finally { await rm(dir, { recursive: true, force: true }); } +}); From cdf650002fa8fd9f772e58b7e3298d0ca475bc95 Mon Sep 17 00:00:00 2001 From: t Date: Tue, 2 Jun 2026 21:03:40 +0530 Subject: [PATCH 9/9] refactor: keep lit-style as AGENTS guidance, not webjs check rules Drop the prefer-reactive-prop-over-getattribute and prefer-signal-over-state-prop rules and the per-file webjs-check-ignore escape hatch. webjs check is for general correctness (SSR safety, server-only imports, erasable TS); lint-flagging every vanilla pattern is noisy and poor DX. The lit-style-over-vanilla convention stays as agent-docs guidance (the vanilla->lit table and the genuinely-needed exceptions), which is where it belongs. --- agent-docs/lit-muscle-memory-gotchas.md | 23 ++-- packages/server/src/check.js | 120 ------------------ .../server/test/check/inline-ignore.test.js | 68 ---------- ...er-reactive-prop-over-getattribute.test.js | 94 -------------- .../prefer-signal-over-state-prop.test.js | 67 ---------- 5 files changed, 8 insertions(+), 364 deletions(-) delete mode 100644 packages/server/test/check/inline-ignore.test.js delete mode 100644 packages/server/test/check/prefer-reactive-prop-over-getattribute.test.js delete mode 100644 packages/server/test/check/prefer-signal-over-state-prop.test.js diff --git a/agent-docs/lit-muscle-memory-gotchas.md b/agent-docs/lit-muscle-memory-gotchas.md index 9bd20b3c7..6fe882d5c 100644 --- a/agent-docs/lit-muscle-memory-gotchas.md +++ b/agent-docs/lit-muscle-memory-gotchas.md @@ -115,21 +115,14 @@ correct lit form, not a vanilla smell. Reading form values with 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. Two `webjs check` -rules enforce the highest-confidence slips: -`prefer-reactive-prop-over-getattribute` (reading own config via -`this.getAttribute`) and `prefer-signal-over-state-prop` (a `state: true` -reactive property that should be a signal). - -**You can still use a flagged pattern when it is genuinely needed.** Both -the project-level switch and a per-file escape hatch exist: - -- Turn a rule off across the project in `package.json` under - `"webjs": { "conventions": { "prefer-reactive-prop-over-getattribute": false } }`. -- Suppress it for ONE file with a comment, keeping the rule on everywhere - else: `// webjs-check-ignore prefer-reactive-prop-over-getattribute ` - (use `*` to suppress every rule for that file). This is the intended way - to keep a deliberate vanilla call that has no clean lit equivalent. +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()` diff --git a/packages/server/src/check.js b/packages/server/src/check.js index 14400e361..54a08a9ed 100644 --- a/packages/server/src/check.js +++ b/packages/server/src/check.js @@ -119,16 +119,6 @@ export const RULES = [ description: 'Flags genuinely browser-only APIs used in a WebComponent constructor, willUpdate, or render() method. The SSR pipeline instantiates the component, runs willUpdate plus controllers\' hostUpdate, reflects properties, and calls render() to produce HTML, on a server element shim that backs the attribute methods but has no real DOM. So a browser global (document, window, localStorage, sessionStorage, navigator, location, matchMedia, screen, history) or an unshimmed HTMLElement member on `this` (attachShadow, shadowRoot, classList, querySelector, querySelectorAll, getBoundingClientRect, focus, blur, scrollIntoView) touched there throws at SSR time (the isomorphic footgun). The attribute methods (getAttribute/setAttribute/hasAttribute/removeAttribute/toggleAttribute), the event methods (addEventListener/removeEventListener/dispatchEvent), and attachInternals are shim-backed and run server-side, so they are NOT flagged. The flagged APIs belong in connectedCallback() or a lifecycle hook (firstUpdated/updated), which SSR never calls; seed first-paint defaults in the constructor (or derive them in willUpdate) only from server-known inputs (attributes, props). Conservative: only the constructor, willUpdate, and render bodies are scanned, and only direct references, so helper indirection is not flagged (the runtime SSR error covers that case).', }, - { - name: 'prefer-reactive-prop-over-getattribute', - description: - 'webjs components are lit-shaped: read your own config through a reactive property (static properties + declare, read this.x), not by calling this.getAttribute, which is vanilla web-component muscle memory. Flags this.getAttribute(\'name\') with a literal attribute name inside a WebComponent class body. Standard attributes that are not modelled as typed props are allowlisted (class, style, id, is, slot, part, title, lang, dir, role, hidden, tabindex, name, type, value, and any aria-* / data-* name), as are dynamic names (this.getAttribute(variable)) and reads off another element (only this.getAttribute on `this` is flagged). The fix is a reactive property whose camelCase name rides the hyphenated attribute. Reserve getAttribute for reading a different element\'s attribute or a standard attribute with native semantics. The companion hasAttribute pattern is covered by the prose convention. See agent-docs/lit-muscle-memory-gotchas.md.', - }, - { - name: 'prefer-signal-over-state-prop', - description: - 'Flags a `state: true` reactive property in a WebComponent\'s static properties. webjs reserves reactive properties for values that ride an HTML attribute (or arrive via .prop SSR hydration); internal reactive state with no attribute is held in a signal instead (framework invariant 5). lit uses `state: true` for attribute-less internal state, but in webjs that should be a `signal` (an instance signal created in the constructor, or a module-scope signal for shared state), read via signal.get() inside render(). Replace the `state: true` declaration with a signal field.', - }, ]; /** Set of all known rule names for fast lookup. */ @@ -475,47 +465,6 @@ function findBrowserMemberUses(code) { return out; } -// Standard attributes that are NOT modelled as typed reactive props, so -// reading them via getAttribute/hasAttribute is legitimate (not flagged). -const ALLOWED_ATTR_READS = new Set([ - 'class', 'style', 'id', 'is', 'slot', 'part', 'title', 'lang', 'dir', - 'role', 'hidden', 'tabindex', 'name', 'type', 'value', -]); - -/** - * Find `this.getAttribute('name')` calls with a literal, non-allowlisted - * attribute name in a (redacted) WebComponent class body. Dynamic names, - * allowlisted standard attributes, and aria-* / data-* are skipped. Reads off - * another element (e.g. `host.getAttribute`) are not matched because the - * pattern is anchored to `this.`. Only `getAttribute` is matched (the config - * read the lit prop API replaces); `hasAttribute` is covered by the prose - * convention in agent-docs/lit-muscle-memory-gotchas.md. - * - * @param {string} classBody - * @returns {{ method: string, attr: string }[]} - */ -/** `delay-duration` -> `delayDuration` (attribute name to reactive-prop name). */ -function attrToCamel(s) { - return s.replace(/-([a-z])/g, (_, c) => c.toUpperCase()); -} - -function findOwnAttributeReads(classBody) { - const out = []; - const seen = new Set(); - const re = /\bthis\.(getAttribute)\(\s*(['"])([^'"]+)\2/g; - let m; - while ((m = re.exec(classBody)) !== null) { - const method = m[1]; - const attr = m[3].toLowerCase(); - if (ALLOWED_ATTR_READS.has(attr) || attr.startsWith('aria-') || attr.startsWith('data-')) continue; - const key = `${method}:${attr}`; - if (seen.has(key)) continue; - seen.add(key); - out.push({ method, attr }); - } - return out; -} - /** * Scan a webjs app directory and report convention violations. * @@ -711,48 +660,6 @@ export async function checkConventions(appDir, opts) { } } - // --- Rule: prefer-reactive-prop-over-getattribute --- - // Lit-shaped components read their own config through a reactive property - // (this.x), not this.getAttribute('x') / this.hasAttribute('x') (vanilla - // muscle memory). Flags only literal, non-allowlisted attribute names on - // `this`; standard attributes (class/style/id/...), aria-*/data-*, dynamic - // names, and reads off another element are not flagged. - if (isRuleEnabled('prefer-reactive-prop-over-getattribute', overrides)) { - for (const { rel, scan } of files) { - if (!/class\s+\w+\s+extends\s+WebComponent/.test(scan)) continue; - for (const body of extractWebComponentClassBodies(scan)) { - for (const { method, attr } of findOwnAttributeReads(body)) { - violations.push({ - rule: 'prefer-reactive-prop-over-getattribute', - file: rel, - message: `\`this.${method}('${attr}')\` reads own config via a vanilla attribute call. Use a reactive property instead.`, - fix: `Declare \`${attrToCamel(attr)}\` in \`static properties\` with a \`declare ${attrToCamel(attr)}\` field (the prop rides the \`${attr}\` attribute), then read \`this.${attrToCamel(attr)}\`. Reserve getAttribute/hasAttribute for reading another element's attribute or a standard attribute with native semantics.`, - }); - } - } - } - } - - // --- Rule: prefer-signal-over-state-prop --- - // `state: true` is a reactive property with no attribute (lit's internal - // state). webjs holds attribute-less internal state in a signal (invariant - // 5), so flag `state: true` in a WebComponent's static properties. - if (isRuleEnabled('prefer-signal-over-state-prop', overrides)) { - for (const { rel, scan } of files) { - if (!/class\s+\w+\s+extends\s+WebComponent/.test(scan)) continue; - for (const body of extractWebComponentClassBodies(scan)) { - if (/\bstate\s*:\s*true\b/.test(body)) { - violations.push({ - rule: 'prefer-signal-over-state-prop', - file: rel, - message: `A \`state: true\` reactive property holds internal state with no attribute. webjs uses a signal for that.`, - fix: `Remove the \`state: true\` declaration and hold the value in a signal: create an instance signal in the constructor (or a module-scope signal for shared state) and read it via signal.get() inside render(). Reactive properties are reserved for values that ride an HTML attribute.`, - }); - } - } - } - } - // --- Rule: no-server-env-in-components --- // Catches `process.env.X` reads in component files where X is not a // WEBJS_PUBLIC_* var and not NODE_ENV. The SSR shim only exposes those @@ -1197,33 +1104,6 @@ export async function checkConventions(appDir, opts) { } } - // Inline opt-out (the per-occurrence escape hatch). A file may suppress a - // rule with a `// webjs-check-ignore [,]` comment (or `* ` for - // all rules) anywhere in the file. The package.json `webjs.conventions` - // switch turns a rule off across the whole project; this is the finer, - // per-file escape hatch so a component that genuinely needs a flagged - // pattern (a vanilla DOM call with no lit equivalent, a deliberate browser - // global) can keep it without disabling the rule everywhere. Authors should - // add a short reason after the rule name. - const ignoresByRel = new Map(); - for (const { rel, content } of files) { - const ignored = new Set(); - const re = /webjs-check-ignore[ \t]+([^\n]+)/gi; - let m; - while ((m = re.exec(content)) !== null) { - for (const tok of m[1].split(/[\s,]+/)) { - if (tok === '*' || RULE_NAMES.has(tok)) ignored.add(tok); - } - } - if (ignored.size) ignoresByRel.set(rel, ignored); - } - if (ignoresByRel.size) { - return violations.filter((v) => { - const ig = ignoresByRel.get(v.file); - return !ig || !(ig.has(v.rule) || ig.has('*')); - }); - } - return violations; } diff --git a/packages/server/test/check/inline-ignore.test.js b/packages/server/test/check/inline-ignore.test.js deleted file mode 100644 index 2778d1f4b..000000000 --- a/packages/server/test/check/inline-ignore.test.js +++ /dev/null @@ -1,68 +0,0 @@ -/** - * The per-file inline escape hatch: `// webjs-check-ignore ` suppresses - * that rule for the file, so a component that genuinely needs a flagged - * pattern can keep it without disabling the rule project-wide. `*` suppresses - * all rules for the file. - */ -import { test } from 'node:test'; -import assert from 'node:assert/strict'; -import { mkdtemp, mkdir, writeFile, rm } from 'node:fs/promises'; -import { join } from 'node:path'; -import { tmpdir } from 'node:os'; - -import { checkConventions } from '../../src/check.js'; - -async function appWith(rel, contents) { - const dir = await mkdtemp(join(tmpdir(), 'webjs-ig-')); - const filePath = join(dir, rel); - await mkdir(filePath.slice(0, filePath.lastIndexOf('/')), { recursive: true }); - await writeFile(filePath, contents); - return dir; -} -const ofRule = (vs, rule, file) => vs.filter((v) => v.rule === rule && v.file.includes(file)); - -const FLAGGED = ` -import { WebComponent, html } from '@webjsdev/core'; -export class C extends WebComponent { - show() { return Number(this.getAttribute('delay-duration') ?? 700); } - render() { return html\`

\`; } -} -C.register('c-el'); -`; - -test('without the directive, the rule fires', async () => { - const dir = await appWith('components/c.ts', FLAGGED); - try { - assert.equal(ofRule(await checkConventions(dir), 'prefer-reactive-prop-over-getattribute', 'c.ts').length, 1); - } finally { await rm(dir, { recursive: true, force: true }); } -}); - -test('webjs-check-ignore suppresses that rule for the file', async () => { - const dir = await appWith('components/c.ts', - `// webjs-check-ignore prefer-reactive-prop-over-getattribute reading a legacy attr\n${FLAGGED}`); - try { - assert.equal(ofRule(await checkConventions(dir), 'prefer-reactive-prop-over-getattribute', 'c.ts').length, 0, - 'the inline directive suppresses the violation'); - } finally { await rm(dir, { recursive: true, force: true }); } -}); - -test('webjs-check-ignore * suppresses all rules for the file', async () => { - const dir = await appWith('components/c.ts', `// webjs-check-ignore *\n${FLAGGED}`); - try { - assert.equal(ofRule(await checkConventions(dir), 'prefer-reactive-prop-over-getattribute', 'c.ts').length, 0); - } finally { await rm(dir, { recursive: true, force: true }); } -}); - -test('the directive only affects the file it is in', async () => { - const dir = await appWith('components/c.ts', FLAGGED); - // a second flagged file without the directive still fires - const fp = join(dir, 'components/d.ts'); - await writeFile(fp, FLAGGED.replace(/\bC\b/g, 'D').replace('c-el', 'd-el')); - await writeFile(join(dir, 'components/c.ts'), - `// webjs-check-ignore prefer-reactive-prop-over-getattribute\n${FLAGGED}`); - try { - const vs = await checkConventions(dir); - assert.equal(ofRule(vs, 'prefer-reactive-prop-over-getattribute', 'c.ts').length, 0, 'c.ts suppressed'); - assert.equal(ofRule(vs, 'prefer-reactive-prop-over-getattribute', 'd.ts').length, 1, 'd.ts still fires'); - } finally { await rm(dir, { recursive: true, force: true }); } -}); diff --git a/packages/server/test/check/prefer-reactive-prop-over-getattribute.test.js b/packages/server/test/check/prefer-reactive-prop-over-getattribute.test.js deleted file mode 100644 index e0587515d..000000000 --- a/packages/server/test/check/prefer-reactive-prop-over-getattribute.test.js +++ /dev/null @@ -1,94 +0,0 @@ -/** - * Tests for the prefer-reactive-prop-over-getattribute rule. webjs components - * are lit-shaped: read own config through a reactive property, not via the - * vanilla this.getAttribute('name'). The rule flags literal, non-allowlisted - * own-attribute reads and steers to a reactive prop. - */ -import { test } from 'node:test'; -import assert from 'node:assert/strict'; -import { mkdtemp, mkdir, writeFile, rm } from 'node:fs/promises'; -import { join } from 'node:path'; -import { tmpdir } from 'node:os'; - -import { checkConventions } from '../../src/check.js'; - -async function appWith(rel, contents) { - const dir = await mkdtemp(join(tmpdir(), 'webjs-rp-')); - const filePath = join(dir, rel); - await mkdir(filePath.slice(0, filePath.lastIndexOf('/')), { recursive: true }); - await writeFile(filePath, contents); - return dir; -} -const find = (vs, file) => vs.filter((v) => v.rule === 'prefer-reactive-prop-over-getattribute' && v.file.includes(file)); - -test('flags this.getAttribute for own config in a component', async () => { - const dir = await appWith('components/tip.ts', ` -import { WebComponent, html } from '@webjsdev/core'; -export class Tip extends WebComponent { - show() { - const d = Number(this.getAttribute('delay-duration') ?? 700); - return d; - } - render() { return html\`

\`; } -} -Tip.register('tip-el'); -`); - try { - const v = find(await checkConventions(dir), 'tip.ts'); - assert.ok(v.some((x) => x.message.includes("this.getAttribute('delay-duration')")), 'own-config getAttribute flagged'); - assert.ok(v.some((x) => x.fix.includes('delayDuration')), 'fix names the camelCase reactive prop'); - } finally { await rm(dir, { recursive: true, force: true }); } -}); - -test('does NOT flag allowlisted standard attributes (class) or aria-/data-', async () => { - const dir = await appWith('components/ok.ts', ` -import { WebComponent, html } from '@webjsdev/core'; -export class Ok extends WebComponent { - render() { - const c = this.getAttribute('class') ?? ''; - const a = this.getAttribute('aria-label'); - const d = this.getAttribute('data-x'); - return html\`

\${c}\${a}\${d}

\`; - } -} -Ok.register('ok-el'); -`); - try { - assert.equal(find(await checkConventions(dir), 'ok.ts').length, 0, 'class / aria-* / data-* must not be flagged'); - } finally { await rm(dir, { recursive: true, force: true }); } -}); - -test('does NOT flag a read off another element, or a dynamic name', async () => { - const dir = await appWith('components/other.ts', ` -import { WebComponent, html } from '@webjsdev/core'; -export class Other extends WebComponent { - _read(host, key) { - const a = host.getAttribute('side'); // another element - const b = this.getAttribute(key); // dynamic name - return [a, b]; - } - render() { return html\`

\`; } -} -Other.register('other-el'); -`); - try { - assert.equal(find(await checkConventions(dir), 'other.ts').length, 0, 'another element + dynamic name must not be flagged'); - } finally { await rm(dir, { recursive: true, force: true }); } -}); - -test('counterfactual control: a reactive-prop component is not flagged', async () => { - const dir = await appWith('components/clean.ts', ` -import { WebComponent, html } from '@webjsdev/core'; -export class Clean extends WebComponent { - static properties = { delayDuration: { type: Number } }; - declare delayDuration; - constructor() { super(); this.delayDuration = 700; } - show() { return this.delayDuration; } - render() { return html\`

\`; } -} -Clean.register('clean-el'); -`); - try { - assert.equal(find(await checkConventions(dir), 'clean.ts').length, 0, 'reading the reactive prop is the correct, unflagged form'); - } finally { await rm(dir, { recursive: true, force: true }); } -}); diff --git a/packages/server/test/check/prefer-signal-over-state-prop.test.js b/packages/server/test/check/prefer-signal-over-state-prop.test.js deleted file mode 100644 index c768e890d..000000000 --- a/packages/server/test/check/prefer-signal-over-state-prop.test.js +++ /dev/null @@ -1,67 +0,0 @@ -/** - * Tests for prefer-signal-over-state-prop. webjs reserves reactive properties - * for attribute-backed values; internal reactive state with no attribute is a - * signal (invariant 5). A `state: true` reactive property is the lit idiom - * webjs replaces with a signal. - */ -import { test } from 'node:test'; -import assert from 'node:assert/strict'; -import { mkdtemp, mkdir, writeFile, rm } from 'node:fs/promises'; -import { join } from 'node:path'; -import { tmpdir } from 'node:os'; - -import { checkConventions } from '../../src/check.js'; - -async function appWith(rel, contents) { - const dir = await mkdtemp(join(tmpdir(), 'webjs-ss-')); - const filePath = join(dir, rel); - await mkdir(filePath.slice(0, filePath.lastIndexOf('/')), { recursive: true }); - await writeFile(filePath, contents); - return dir; -} -const find = (vs, file) => vs.filter((v) => v.rule === 'prefer-signal-over-state-prop' && v.file.includes(file)); - -test('flags a state: true reactive property', async () => { - const dir = await appWith('components/s.ts', ` -import { WebComponent, html } from '@webjsdev/core'; -export class S extends WebComponent { - static properties = { open: { type: Boolean, state: true } }; - declare open; - render() { return html\`

\`; } -} -S.register('s-el'); -`); - try { - assert.equal(find(await checkConventions(dir), 's.ts').length, 1, 'state: true must be flagged'); - } finally { await rm(dir, { recursive: true, force: true }); } -}); - -test('counterfactual: a signal-backed component (no state: true) is not flagged', async () => { - const dir = await appWith('components/sig.ts', ` -import { WebComponent, html, signal } from '@webjsdev/core'; -export class Sig extends WebComponent { - open = signal(false); - render() { return html\`

\${this.open.get()}

\`; } -} -Sig.register('sig-el'); -`); - try { - assert.equal(find(await checkConventions(dir), 'sig.ts').length, 0, 'a signal is the correct, unflagged form'); - } finally { await rm(dir, { recursive: true, force: true }); } -}); - -test('does not flag an attribute-backed reactive prop (reflect, no state)', async () => { - const dir = await appWith('components/attr.ts', ` -import { WebComponent, html } from '@webjsdev/core'; -export class Attr extends WebComponent { - static properties = { open: { type: Boolean, reflect: true } }; - declare open; - constructor() { super(); this.open = false; } - render() { return html\`

\`; } -} -Attr.register('attr-el'); -`); - try { - assert.equal(find(await checkConventions(dir), 'attr.ts').length, 0, 'attribute-backed reactive props are correct'); - } finally { await rm(dir, { recursive: true, force: true }); } -});