Skip to content

The SSR attribute reader ignores converter.fromAttribute #1340

Description

@vivek7405

Line anchors in this body were verified at HEAD 79fc28fc. Several anchors in the previous body had drifted and are corrected below.

Problem

attributeChangedCallback (the client attribute reader) honours a property's custom converter.fromAttribute. applyAttrsToInstance (the SSR attribute reader) does not. A property declared with a converter is therefore read one way during SSR and another way the moment the element upgrades in the browser, which is a hydration divergence for any component using the documented converter option together with a source-markup attribute.

Client, packages/core/src/component.js:1163 (the previous body said :1161, which is stale by two lines). The converter arm runs FIRST, before any type-based coercion:

    let v;
    if (def.converter && def.converter.fromAttribute) {
      v = def.converter.fromAttribute(value, def.type);
    } else if (def.type === Number) {

The full precedence chain is component.js:1162-1201 (let v; through the else { v = value; } fallthrough). This is the reference implementation and its BEHAVIOUR must not change.

SSR, packages/core/src/render-server.js:1703 (applyAttrsToInstance, previously cited as :1700). It resolves the property name at L1710-1717 (previously cited as L1708-1715), normalises the declaration at L1726, then dispatches only on def.type starting at L1727 (previously cited as "around L1723"):

    const def = typeof rawDef === 'object' ? rawDef : { type: rawDef };
    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(unescapeAttr(raw)); } catch { instance[propName] = null; }
    } else instance[propName] = raw;

There is no converter arm anywhere in that function.

Reproduced at HEAD

A read-only script declaring mode: prop(String, { converter: { fromAttribute: (v) => String(v).toUpperCase() } }) and rendering <my-el2 mode="a">, with the client half driven through linkedom, produces these two observed strings:

SSR   : <my-el2 mode="a" data-wj-host><!--webjs-hydrate--><span>mode=a</span></my-el2>
CLIENT: el.mode = "A"

SSR paints a. The upgraded element holds A. The claim in the previous body is confirmed exactly.

A second probe pins the throwing case as it behaves TODAY. A converter that throws does NOT throw at SSR right now, because the arm that would call it does not exist, so <throw-conv v="x"> renders <i>x</i> from the untouched attribute string. After this fix it throws, and that is a real behaviour change worth stating plainly rather than discovering in review.

The converter option is public, documented API

It appears in the PropertyDeclaration typedef at packages/core/src/component.js:86-90 (previously cited as 86-88), whose own prose says fromAttribute is called in attributeChangedCallback, which after this change is no longer the whole truth. It also appears in the root AGENTS.md property-options list at L282, in the .agents/skills/webjs/references/components.md options table at L142, and in the docs site at website/app/docs/components/page.ts.

Every place the gap is currently written down

Four source comments state that the SSR reader has no converter arm. All four become wrong when this lands, and all four must be replaced:

  1. packages/core/src/component.js:1194-1197, inside the Object/Array branch: "Scoped to the default converter. A prop supplying its own converter.fromAttribute is handled by the arm above and the SSR reader has no converter arm at all, so the two sides already read such a prop differently. That gap predates A cyclic value on a JSON-typed reflect:true prop throws from reflection #1253 and is left alone."
  2. packages/core/src/render-server.js:1757-1762, the matching branch: "Scoped to the DEFAULT converter, which is all this branch is. The client reader tries converter.fromAttribute FIRST and only falls through to here when there is none, while this function has no converter arm at all, so a prop declaring one is read differently by the two sides regardless of what this line does. That gap predates A cyclic value on a JSON-typed reflect:true prop throws from reflection #1253 and is left alone rather than widened."
  3. packages/core/test/rendering/reflect-function-guard.test.js:527-528: "This is the default-converter path only: the SSR reader has no fromAttribute arm, so a prop declaring a converter is already read differently either way."
  4. test/bun/reflect-unserializable.mjs:18: "Default-converter path only: the SSR reader has no fromAttribute arm."

The previous body named only the first two.

Where this came from

Found during the review of #1335, which changed the unparseable-JSON fallback in BOTH readers and had to scope its own comments around this gap. The gap predates that PR and is not caused by it.

Design / approach

Two calls, both settled here.

Decision 1: EXTRACT one shared readAttributeValue(def, value, decode), do not bolt an arm on

Settled in favour of extraction. Four pieces of evidence, in order of weight.

Lit does exactly this, and WebJs deliberately matches lit's component runtime. @lit-labs/ssr has no second attribute reader. packages/labs/ssr/src/lib/lit-element-renderer.ts:188 forwards its attributeChangedCallback straight into the element's own reader:

const {attributeToProperty, changedProperties} = _$LE;

  override attributeChangedCallback(name, _old, value) {
    attributeToProperty(this.element as LitElement, name, value);
  }

attributeToProperty is ReactiveElement.prototype._$attributeToProperty (packages/reactive-element/src/reactive-element.ts:1233), exposed to the SSR package through packages/lit-element/src/private-ssr-support.ts:22 precisely so the server pass runs the BROWSER's reader rather than a copy of it. Lit hit this class of problem and solved it structurally. An arm-only fix here leaves WebJs with the copy lit refused to keep.

The readers have already drifted twice, and #1341 names three more. The two settled drifts are this converter gap and the unparseable-JSON fallback fixed in #1253. Both were found late, both by review rather than by a test, and both needed the same edit applied twice in sync. Two independent drifts in one function is the standard threshold for removing the class rather than the instance.

The hot-path objection does not survive measurement. applyAttrsToInstance runs per component instance per render, so the concern was real and was measured rather than asserted. The repo has no SSR render benchmark (scripts/bench-listener.mjs is the only bench script and it measures the listener), so one was written read-only in the scratchpad. It renders 2000 components carrying 4 attributes each (8000 attribute reads per render) and separately times the inline branch chain against the same chain behind one function call, 5 million reads each, three runs.

Measurement Value
renderToString of 2000 components, 4 attributes each 148 ms per render
Inline branch chain 24 to 29 ns per read
Extracted helper (one extra call) 25 to 29 ns per read
Delta across three runs -3.53, +4.17, +0.87 ns per read
All 8000 reads as a share of one render about 0.14%
The extraction's delta as a share of one render at most 0.023%, sign unstable

The delta changes sign between runs, so it is noise at this resolution, and even taking the worst run at face value the extraction costs 0.023% of a render. V8 inlines a small monomorphic function; the branch chain, not the call, is the cost. The objection is refuted.

The extraction is provably behaviour-neutral for the SSR side. The two chains differ only in the null guards the client carries (value == null ? null : Number(value) against the SSR Number(raw), and the same for the JSON branch) and in the JSON branch's unescapeAttr. The SSR reader's input always comes from parseAttrs, which yields a string for every attribute including a bare one (''), never null, so adopting the client's guarded body verbatim changes nothing for any input SSR can produce. Checked branch by branch: Number('') is 0 either way, '' !== 'false' is true either way, JSON.parse('') throws to null either way.

Where the helper lives, and why that direction is safe. In packages/core/src/component.js, exported, imported BY packages/core/src/render-server.js. This matches lit's structure (the reader lives with the element, the SSR renderer reaches for it) and the previous body's own instinct, and it was verified rather than assumed:

  • No cycle. Nothing under packages/core/src/ statically imports render-server.js. The only references are the two dynamic await import('./render-server.js') calls in src/testing.js and prose in comments. component.js does not import it directly or transitively.
  • No new module in any bundle. component.js is already exported from BOTH packages/core/index.js:11 and packages/core/index-browser.js:29, so it is already in the Node graph and the browser graph. render-server.js is in neither browser graph (index-browser.js strips it by design, see its header comment). The new edge therefore adds nothing to the browser bundle and nothing new to the Node bundle.
  • No browser-only code pulled server-side. The only modules the @webjsdev/core/server export condition newly reaches are component.js and its transitive render-client.js, action-abort-client.js, and signal.js. All three already load in a bare Node process, verified directly: node -e "import('./packages/core/src/component.js')", the same for render-client.js, and import('@webjsdev/core/server') all load clean. And component.js is already loaded by anything that SSRs a component, since the pipeline does new Cls() on a WebComponent subclass.

Alternative rejected: the converter arm alone. Four lines, zero risk to the other branches, and it fixes the reported symptom. Rejected because it leaves two hand-maintained parallel chains whose drift record is now 2 for 2 with three more cases open in #1341, and because the measurement removed the only argument that favoured it.

Alternative rejected: a new zero-import leaf module such as packages/core/src/attribute-reader.js, imported by both. Genuinely attractive: it would also be the natural home for the hyphenate and camelCase helpers that are currently duplicated in both files. Rejected for this issue because moving those two helpers is a rename churn across both files that has nothing to do with the converter, and because putting the reader anywhere other than beside the declaration semantics it interprets is a worse default than lit's. If #1341's name-resolution work ends up wanting a shared module for hyphenate / camelCase, that is #1341's call to make, and readAttributeValue can move with them at that point.

Alternative rejected: mirroring the arm and adding a drift test that asserts the two chains agree over a value matrix. Rejected because such a test can only cover inputs someone thought of, which is exactly how both existing drifts got through, and because it costs more code than the extraction it substitutes for.

Decision 2: a throwing converter.fromAttribute PROPAGATES, on both readers, unguarded

Settled in favour of propagating. Do not catch, do not warn, do not fall back.

The repo's own precedent points here, not the other way. #1253's unserializable-reflection guard is the obvious analogy, and reading its code rather than its issue text settles the question against catching. _reflectAttribute at packages/core/src/component.js:751 puts its converter branch FIRST, at L759-761, and both guards AFTER it, with the reason stated in the source at L773-775:

        // Placed AFTER the converter branch on purpose: a custom
        // `toAttribute` is author-controlled, so its author has taken
        // responsibility for serializing whatever they are handed.

So the house pattern is not "guard everything that can throw". It is "guard the framework's OWN default serialization, and leave an author-supplied converter alone". #1253 caught a JSON.stringify the framework performed on the author's behalf, on a value the author never asked to be stringified. A throwing fromAttribute is the author's own function throwing. Following the precedent means NOT catching it.

Lit does not guard it either. reactive-element.ts:1250 calls converter.fromAttribute!(value, options.type) with no try/catch, and because @lit-labs/ssr routes through that same function, lit propagates on both sides for the same reason.

Catching on one side manufactures a new divergence. The client half is out of scope by the issue's own terms and by lit parity, so a catch could only be added at SSR. That would give a page whose SSR paint holds a fallback value while the browser upgrade throws and keeps the constructor value, which is strictly worse than both sides failing. The whole point of this issue is agreement.

What actually happens on each side, stated so it can be pinned by a test. At SSR the throw lands in the per-component error isolation catch at packages/core/src/render-server.js:1100. applyAttrsToInstance is called at L973, inside the try that opens at L947, so the throw is caught there: the loop continues and siblings render, the server logs [webjs] SSR failed for <tag> with the converter's own stack, and the element renders renderError() if the component defines one, the dev error box in dev, or an EMPTY element at a 200 in production. On the client the throw escapes attributeChangedCallback during upgrade, the browser reports it uncaught, and the property keeps its constructor value. The two sides do not agree on OUTPUT when the converter throws, and that is accepted deliberately: the agreement this issue buys is over every converter that does not throw.

Rejected: catch at SSR, warn, and fall back to the type-based coercion. It reproduces the exact silent-empty-component-at-200 shape #1253 removed, which is the strongest argument for it. Rejected anyway, because the fallback value would be one no reader on either side would otherwise produce, it contradicts the stated house rule at component.js:773-775, it diverges from lit, and it hides an author bug behind a warning nobody reads in production. The production failure is loud in the server log and visible as a missing component; a silently wrong value is worse.

Rejected: decorate the thrown error with the property name and tag before re-throwing. Appealing, and cheap. Rejected because the decoration would have to live in the shared helper to reach both readers, which changes what the client throws and therefore changes attributeChangedCallback behaviour, which this issue forbids. The existing [webjs] SSR failed for <tag> log already names the tag and prints the converter's own stack, which points at the author's function.

Decision 2b: a converter returning a value SSR cannot serialise needs NO new guard

A converter may return a DOM node, a function, a Symbol, or a cyclic object. Checked empirically at HEAD, and the existing house guards already cover every path where it matters:

  • Not reflected. The value is only a property, used by render(). A function in a text hole renders as nothing: <fn-conv v="x" data-wj-host><!--webjs-hydrate--><i></i></fn-conv>. No source leak, no throw.
  • Reflected. _reflectAttribute runs on the SSR side too (from performServerUpdate), and its typeof value === 'function' guard (A reflect:true prop stringifies a function, leaking its source #1169, component.js:763-778) and its JSON.stringify guard (A cyclic value on a JSON-typed reflect:true prop throws from reflection #1253, component.js:783-817) already fire on whatever the converter returned. Verified: a reflected function value renders <r-fn data-wj-host>…<i>ok</i></r-fn> with the attribute removed and the warning [webjs] reflect:true property "v" on <r-fn> holds a function (or an array carrying one) ….

So the answer is that SSR treats a converter's return exactly as it treats any other property value, and the guard set established by #1169 / #1253 / #1335 is the house pattern already in place. Add no new guard. Say this in the source comment so the next reader does not re-open it.

What #1341 inherits from this decision

After this issue lands there is ONE attribute reader, readAttributeValue(def, value, decode) in packages/core/src/component.js, and both attributeChangedCallback and applyAttrsToInstance become thin callers of it: each resolves a property NAME, normalises the declaration to a { type, … } object, then hands that pair to the shared reader. That split is the boundary #1341 works against. Its two name-resolution cases (a state: true prop the SSR reader reads that observedAttributes at packages/core/src/component.js:579 filters out on the client, and a camelCase source attribute the SSR reader resolves that the browser lowercases away) live entirely in the CALLERS, above the shared reader, and #1341 fixes them there without touching readAttributeValue at all. Its third case, unescapeAttr at packages/core/src/render-server.js:1799 decoding only &lt;, &quot;, and &amp; where a browser decodes every entity, lands on the decode parameter this issue introduces. This issue passes unescapeAttr from applyAttrsToInstance and the shared reader applies it to the JSON branch ONLY, exactly where the SSR reader applies it today, so the extraction changes no behaviour. #1341 owns the two open questions about that seam: whether the decoder should be full rather than three-entity, and whether it belongs on every branch rather than only the JSON one. If #1341 answers yes to both, the seam collapses, applyAttrsToInstance decodes once at its call site, the third parameter is deleted, and the shared reader's signature becomes readAttributeValue(def, value). None of that is pre-empted here.

Implementation plan

Ordered. Every call is decided; nothing below is optional.

Step 1. Add the shared reader to packages/core/src/component.js

Insert a new exported function immediately after defaultHasChanged (which ends at L101, just before the String(v) helper's JSDoc at L103). It is a module-level function declaration, so its position relative to the class does not matter for hoisting; put it there so it sits with the other declaration-semantics helpers.

/**
 * Read one attribute string into its declared property value.
 *
 * THE attribute reader, singular. `attributeChangedCallback` below and
 * `applyAttrsToInstance` in `render-server.js` are both thin callers of this
 * function, so the client and the SSR pass cannot drift on precedence or on a
 * fallback again. They had drifted twice before this existed: the
 * unparseable-JSON fallback (#1253) and a missing converter arm (#1340).
 *
 * lit is built the same way. `@lit-labs/ssr`'s LitElementRenderer forwards its
 * `attributeChangedCallback` into the element's own `_$attributeToProperty`
 * through `lit-element/private-ssr-support.js`, so its server pass runs the
 * browser's reader rather than a copy of it.
 *
 * The caller owns NAME resolution (which attribute maps to which property) and
 * declaration normalisation; this function owns only the value. That split is
 * deliberate: the two callers reach an attribute by different routes and do NOT
 * see the same attribute set, which is a separate problem tracked in #1341.
 *
 * @param {PropertyDeclaration} def normalised declaration (`{ type, … }`)
 * @param {string|null} value the attribute text
 * @param {(s: string) => string} [decode] applied to the JSON branch's input
 *   only. The client is handed a value the DOM already decoded and passes
 *   nothing; the SSR reader walks the raw source tag and passes `unescapeAttr`.
 *   Applied only where the SSR reader applied it before this function existed,
 *   so the extraction changes no behaviour. Whether the decode belongs on every
 *   branch, and whether three entities is enough, are #1341's questions.
 * @returns {unknown}
 */
export function readAttributeValue(def, value, decode) {
  if (def.converter && def.converter.fromAttribute) {
    // Deliberately UNGUARDED, and deliberately first (#1340). An author who
    // supplies a converter owns the whole read, which is the same rule
    // `_reflectAttribute` already states for `toAttribute`: its guards sit
    // AFTER the converter branch because a custom converter is
    // author-controlled. So a converter that throws throws, on both sides. At
    // SSR that lands in per-component error isolation (render-server.js), which
    // surfaces an error box in dev and an empty element at a 200 in prod with
    // the cause in the server log; on the client it escapes
    // attributeChangedCallback during upgrade. Catching on one side only would
    // manufacture a fresh divergence (an SSR paint holding a fallback against a
    // browser holding the constructor value), which is worse than both sides
    // failing. lit propagates too (reactive-element `_$attributeToProperty`).
    //
    // A converter RETURN that SSR cannot serialise needs no guard here either.
    // Unreflected it is only a property, and a function in a text hole renders
    // as nothing. Reflected, the #1169 function guard and the #1253
    // unserializable guard in `_reflectAttribute` already run on the SSR side
    // and cover whatever the converter produced.
    return def.converter.fromAttribute(value, def.type);
  }
  if (def.type === Number) return value == null ? null : Number(value);
  if (def.type === Boolean) return value != null && value !== 'false';
  if (def.type === Object || def.type === Array) {
    // An attribute that is not parseable JSON yields `null` rather than the raw
    // string (#1253), because a STRING is never a valid value for a property
    // the author declared `Object` or `Array`, whatever put it there. lit's
    // `defaultConverter.fromAttribute` lands on the same `null`, for the reason
    // its own comment gives: an element does not complain about being
    // mis-configured. Both readers reach this line, so they cannot disagree.
    //
    // An attribute that was never PRESENT does not reach either reader, so such
    // a prop simply keeps its constructor value.
    //
    // This is about the FALLBACK, not a guarantee that the two readers see the
    // same attributes in the first place. They reach an attribute by different
    // routes (the client via `observedAttributes` and the browser's own name
    // lowercasing, the SSR one by walking the parsed source tag), and
    // hand-written markup can land in the gaps between those routes. Those gaps
    // are tracked in #1341 and are not enumerated here.
    if (value == null) return null;
    try { return JSON.parse(decode ? decode(value) : value); } catch { return null; }
  }
  return value;
}

The def.converter.fromAttribute arm, the four type branches, and the fallthrough are the client chain at component.js:1162-1201 moved verbatim, with the JSON branch's input routed through the optional decode.

Step 2. Make attributeChangedCallback call it

packages/core/src/component.js:1162-1201. Today:

    let v;
    if (def.converter && def.converter.fromAttribute) {
      v = def.converter.fromAttribute(value, def.type);
    } else if (def.type === Number) {
      v = value == null ? null : Number(value);
    } else if (def.type === Boolean) {
      v = value != null && value !== 'false';
    } else if (def.type === Object || def.type === Array) {
      // … 30 lines of comment, including the four-line "the SSR reader has no
      // converter arm at all" paragraph at L1194-1197 …
      try { v = value == null ? null : JSON.parse(value); } catch { v = null; }
    } else {
      v = value;
    }

    if (this[propName] !== v) {

After. The whole chain and every comment in it moves into readAttributeValue, and the stale L1194-1197 paragraph is DELETED rather than moved (Step 1's comment replaces it):

    // One reader for both sides (#1340). `applyAttrsToInstance` in
    // `render-server.js` calls the same function, so precedence and every
    // fallback are shared rather than mirrored. The client is handed a value
    // the DOM already decoded, so it passes no `decode`.
    const v = readAttributeValue(def, value);

    if (this[propName] !== v) {

Nothing above L1162 changes. The name resolution at L1149-1161 stays exactly as it is; it is #1341's territory.

Step 3. Make applyAttrsToInstance call it

packages/core/src/render-server.js. Add the import as a new line 19, after import { cspNonce } from './csp-nonce.js'; (line 18, currently the last import):

import { readAttributeValue } from './component.js';

Then replace L1727-1764. Today:

    const def = typeof rawDef === 'object' ? rawDef : { type: rawDef };
    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) {
      // … 34 lines of comment, including the six-line "this function has no
      // converter arm at all" paragraph at L1757-1762 …
      try { instance[propName] = JSON.parse(unescapeAttr(raw)); } catch { instance[propName] = null; }
    } else instance[propName] = raw;

After:

    const def = typeof rawDef === 'object' ? rawDef : { type: rawDef };
    // One reader for both sides (#1340): `readAttributeValue` in `component.js`
    // is the same function `attributeChangedCallback` calls, so a custom
    // `converter.fromAttribute` now runs here too, ahead of type coercion, and
    // the #1253 unparseable-JSON fallback is shared rather than mirrored.
    //
    // `raw` is the entity-encoded attribute text (`parseAttrs` returns the
    // literal characters between the quotes), so the JSON branch needs the
    // entities decoded before `JSON.parse`: a JSON attribute carries `&quot;`
    // for every `"`, and parsing it raw throws. The client needs no decode,
    // because the DOM already did it. `unescapeAttr` reverses far less than a
    // browser does, and whether it should be full, and whether it belongs on
    // every branch rather than only this one, are #1341's questions.
    //
    // A converter that THROWS is not caught here. It lands in the
    // per-component error isolation below, which is deliberate: an author who
    // supplies a converter owns the read, the same rule `_reflectAttribute`
    // states for `toAttribute`. See the comment on `readAttributeValue`.
    instance[propName] = readAttributeValue(def, raw, unescapeAttr);

unescapeAttr is a hoisted function declaration at L1799, so referencing it at L1727 is fine.

Behaviour check to state in the PR description, since the reviewer will want it. The SSR reader's input always comes from parseAttrs, which yields a string for every attribute including a bare one ('') and never null, so the client's null guards are unreachable from this call site and every branch produces the identical value it produced before: Number('') is 0, '' !== 'false' is true, JSON.parse('') throws to null.

Step 4. Declare the new export in packages/core/src/component.d.ts

packages/core/package.json publishes a ./component export condition whose types is ./src/component.d.ts, so the runtime export needs a matching declaration or the two surfaces disagree. Add after the prop overloads (the file ends at L215):

export declare function readAttributeValue(
  def: PropertyDeclaration,
  value: string | null,
  decode?: (s: string) => string,
): unknown;

Do NOT re-export it from packages/core/index.js or index-browser.js. It is an internal seam between two framework modules, not app-facing API.

Step 5. Update the two remaining stale comments outside src/

  • packages/core/test/rendering/reflect-function-guard.test.js:527-528. Replace "This is the default-converter path only: the SSR reader has no fromAttribute arm, so a prop declaring a converter is already read differently either way." with "Both readers now call one shared readAttributeValue (The SSR attribute reader ignores converter.fromAttribute #1340), so this fallback is shared rather than mirrored; the converter arm sits ahead of it and is covered separately in ssr-prop-options.test.js."
  • test/bun/reflect-unserializable.mjs:18. Replace "Default-converter path only: the SSR reader has no fromAttribute arm." with "Default-converter path only; the converter arm both readers now share (The SSR attribute reader ignores converter.fromAttribute #1340) is covered by test/bun/attribute-converter-parity.mjs."

Tests

Unit, SSR side: packages/core/test/rendering/ssr-prop-options.test.js (extend)

This file, not component-lifecycle.test.js. Correcting the previous body: component-lifecycle.test.js runs under linkedom and never imports renderToString, so an SSR assertion does not belong there. ssr-prop-options.test.js already imports WebComponent, prop, html, and renderToString (L7-9) and its stated charter is SSR coverage of reactive-prop options, which is exactly this. Add four tests:

  1. converter.fromAttribute runs at SSR, ahead of type coercion. Declare mode: prop(String, { converter: { fromAttribute: (v) => String(v).toUpperCase() } }) on a probe that renders ${this.mode}, render <ssr-converter mode="a">, assert the output matches />A</ and does NOT contain >a<. This is the counterfactual: deleting the two-line converter arm from readAttributeValue reds it with the observed >a< from the repro above.
  2. the converter wins over the declared type at SSR, matching the client. Declare v: prop(Object, { converter: { fromAttribute: (v) => ({ raw: v }) } }), the exact declaration the existing client test at packages/core/test/lifecycle/component-lifecycle.test.js:101 uses, render <ssr-converter-obj v="abc">, assert the rendered JSON is {"raw":"abc"} and not null. Pairs with that client test by construction, so the two files pin the same declaration on both sides.
  3. a throwing converter is not caught by the reader, so the component renders its SSR error state. Pins Decision 2. Capture console.error across the render, assert the emitted element does not contain the probe's own content and that a [webjs] SSR failed for <…> line was logged. Do not assert on the error-box markup, which is dev-surface detail.
  4. a prop with NO converter is byte-identical to what SSR emitted before the extraction. One probe declaring s: String, n: Number, b: Boolean, o: prop(Object), a: prop(Array), rendered against a fixed source tag including a bare boolean attribute, an unparseable JSON attribute, an empty-string attribute, and an entity-encoded JSON attribute. Assert the exact expected output string. This is the neutrality proof for the extraction, and it is the test that fires if someone later "simplifies" a null guard away.

Unit, client side: packages/core/test/lifecycle/component-lifecycle.test.js (extend)

The existing test at L101-109 stays untouched and is the reference:

test('custom converter.fromAttribute overrides type-based coercion', () => {
  class C extends WebComponent({
    v: prop(Object, { converter: { fromAttribute: (v) => ({ raw: v }) } }),
  }) {}
  C.register('custom-from');
  const el = document.createElement('custom-from');
  el.attributeChangedCallback('v', null, 'abc');
  assert.deepEqual(el.v, { raw: 'abc' });
});

Add one test beside it: a throwing converter.fromAttribute propagates out of attributeChangedCallback, using assert.throws. It pins the client half of Decision 2 so a later well-meaning try/catch on either side reds a test rather than silently making the two sides disagree.

Browser: packages/core/test/rendering/browser/reflect-function-guard.test.js (extend)

This file over ssr-client-parity.test.js. It already carries the #1253 READ-side suite at L315-345, whose header comment states the reason a browser is required: only a browser calls attributeChangedCallback on upgrade, so a node test calling it by hand "exercises the branch but not the path the divergence lived on". #1340 is the same mechanism one arm earlier, so it belongs beside it.

Add a suite immediately after L345, modelled line for line on the existing one:

suite('converter.fromAttribute reads identically at SSR and through a real upgrade (#1340)', …) with one test, through a REAL element upgrade, matching what the SSR reader makes of the same markup. Register a module-scope probe near the other probes (around L91) that renders its own prop value so the read is observable in the DOM:

class ConverterReaderProbe extends WebComponent({
  mode: prop(String, { converter: { fromAttribute: (v) => String(v).toUpperCase() } }),
}) {
  render() { return html`<i>mode=${this.mode}</i>`; }
}
ConverterReaderProbe.register('converter-read-probe');

The test sets host.innerHTML = '<converter-read-probe mode="a"></converter-read-probe>', awaits customElements.whenDefined then el.updateComplete, asserts el.mode === 'A' and that el.textContent includes mode=A, then renders the SAME markup through renderToString and asserts the SSR string also includes mode=A. The final assertion is the agreement assertion and is the one that reds on a revert. Run with npm run test:browser.

Bun: new pair, test/bun/attribute-converter-parity.mjs and test/bun/attribute-converter-parity.test.mjs

Required, and confirmed required rather than assumed. .claude/hooks/require-bun-parity-with-runtime-src.sh matches staged packages/*/src paths against a regex whose alternatives include render-server, and this change stages packages/core/src/render-server.js, so the hook BLOCKS the commit unless a test/bun/** file is staged alongside. (component is not in that regex, so it is render-server.js alone that triggers it.)

Follow test/bun/reflect-unserializable.mjs exactly, which is the model the previous body named and which is correct. It is a plain assert script, NOT *.test.mjs, so the root node --test runner does not double-run it; it imports html, WebComponent, prop from the bare @webjsdev/core and renderToString from @webjsdev/core/server, stamps every assertion message with a runtime string built from process.versions.bun ?? process.versions.node, and is run from the repo root so the bare specifier resolves to the workspace package. Assert three things:

  1. A converter.fromAttribute is applied by the SSR reader under whichever runtime is executing (mode="a" renders A).
  2. The converter beats the declared type (the prop(Object) plus { raw: v } declaration renders {"raw":"abc"}, not null).
  3. A throwing converter surfaces as an isolated component with the SSR-failure log, and the sibling component in the same render still renders. This is the runtime-sensitive half worth pinning per runtime, since it is the interaction between the reader and the isolation catch.

Add the two-line test/bun/attribute-converter-parity.test.mjs wrapper copying test/bun/reflect-unserializable.test.mjs verbatim in shape, so the root node --test run exercises the Node path and node scripts/run-bun-tests.js auto-discovers the Bun path.

Run and report all three:

node test/bun/attribute-converter-parity.mjs
bun  test/bun/attribute-converter-parity.mjs
node scripts/run-bun-tests.js

Layers that do NOT apply

  • e2e (test/e2e/*.test.mjs under WEBJS_E2E=1). The change is confined to one pure function inside renderToString. There is no route, no network path, no navigation, no streaming, and no client-router involvement. The only user-observable surface is the SSR-versus-upgrade agreement, which the browser layer asserts directly through a real element upgrade, which is strictly stronger than an e2e page assertion of the same thing.
  • Smoke (test/examples/*/smoke/*). No scaffolded-app or example-app surface changes. No template, generator, or gallery demo declares a converter.
  • webjs check. No rule is added or changed. See Out of scope for why no-browser-globals-in-render is deliberately left alone.

Counterfactual summary

  • Delete the two-line converter arm from readAttributeValue: reds ssr-prop-options.test.js tests 1 and 2, the browser agreement suite, and Bun assertions 1 and 2.
  • Wrap the converter call in a try/catch with a fallback: reds ssr-prop-options.test.js test 3, the component-lifecycle.test.js throw test, and Bun assertion 3.
  • Restore the two inline chains and drop the shared helper: every no-converter test stays green, which is the intended proof that the extraction is behaviour-neutral, and the converter tests red, which is the intended proof that the fix and the extraction are separable claims.

Docs

Every surface below is required. The task is not done until all of them are in sync.

  • website/app/docs/components/page.ts, the "Attribute-to-Property Coercion" list at L131-137 (the previous body's "around L120-130" is off by about ten lines). Add one paragraph immediately after the closing </ul> at L137, before the camelCase paragraph at L139: a property supplying its own converter.fromAttribute bypasses the list entirely, the converter runs first and its return value IS the property value, it runs on BOTH readers so it executes during SSR as well as on the browser upgrade, it must therefore avoid browser globals (document, window, navigator) because SSR has no DOM and the component would render its error state instead of its content, and a converter that throws is not caught on either side because an author who writes one owns the conversion.
  • .agents/skills/webjs/references/components.md, the options table converter row at L142. Change the Meaning cell to state that fromAttribute runs on BOTH readers (client upgrade and SSR) ahead of type coercion. Then extend the sentence at L144 ("For anything the built-in converters cannot parse (Date, Map, Set) supply a converter") with the same three facts as the docs-site paragraph: it runs server-side, keep it free of browser globals, a throw is not caught. Add it as its own short paragraph rather than lengthening L144.
  • Root AGENTS.md, the property-options sentence in the reactive-properties paragraph at L282. After "Property options: type (default String), reflect, state, hasChanged, converter, default, attribute.", add one sentence: a converter.fromAttribute runs on both the SSR reader and the client reader, ahead of type coercion, so it executes server-side and must not touch browser globals, and a throw from it is not caught on either side.
  • packages/core/src/component.js:86-90, the PropertyDeclaration typedef. Its text currently says "fromAttribute is called in attributeChangedCallback", which stops being the whole truth. Change to state that fromAttribute is called by both attribute readers (the client attributeChangedCallback and the SSR applyAttrsToInstance, through the shared readAttributeValue), so it runs server-side too.
  • The four source comments enumerated under Problem, replaced per Steps 1, 2, 3, and 5.
  • packages/core/src/component.d.ts, per Step 4.
  • Changelog. No hand-written changelog file: WebJs generates changelog/<pkg>/<version>.md from conventional-commit subjects at the version bump (framework-dev.md, "Changelog: per-package, per-version, auto-generated"). So carry it in the commit. Subject: fix: honour converter.fromAttribute in the SSR attribute reader. The body must state the behaviour change for existing apps in plain terms: a converter.fromAttribute that previously ran only in the browser now also runs during SSR, so one that touches document / window / navigator will throw server-side where it did not before, and the component renders its error state.

Not a doc surface, verified. packages/cli/templates/.agents/skills/webjs/references/components.md does not exist in the working tree and needs no lockstep edit. scripts/sync-scaffold-skill.mjs copies the canonical .agents/skills/webjs/ into packages/cli/templates/ at prepack and removes it at postpack, so the canonical file is the single source. README.md needs nothing: converter is not a headline capability and is not mentioned there.

Acceptance criteria

  • There is exactly ONE attribute-value reader in packages/core, readAttributeValue in src/component.js, and both attributeChangedCallback and applyAttrsToInstance call it. Neither file still carries its own type-dispatch chain.
  • A property declaring converter.fromAttribute resolves to the same value at SSR and after a real client upgrade, for the same source markup, asserted in one test that renders both.
  • Converter precedence matches the client exactly: the converter runs before any type-based coercion, on both readers.
  • <my-el mode="a"> with an upper-casing converter SSRs holding A, where at 79fc28fc it SSRs holding a.
  • A throwing converter.fromAttribute is NOT caught by either reader. At SSR it reaches the per-component isolation catch at render-server.js:1100 and the server logs [webjs] SSR failed for <tag>; on the client it escapes attributeChangedCallback. Both halves are pinned by a test, and the decision is recorded in a source comment on the converter arm.
  • A converter returning a non-serialisable value adds no new guard, and the existing _reflectAttribute guards (A reflect:true prop stringifies a function, leaking its source #1169 function, A cyclic value on a JSON-typed reflect:true prop throws from reflection #1253 unserializable) are confirmed to cover the reflected case.
  • A property with NO converter is unaffected, asserted against an exact expected SSR output string covering String, Number, Boolean, Object, and Array, including a bare boolean attribute, an empty-string attribute, an unparseable JSON attribute, and an entity-encoded JSON attribute.
  • The unparseable-JSON fallback from A cyclic value on a JSON-typed reflect:true prop throws from reflection #1253 still yields null on both readers, and the existing tests that pin it are green unchanged.
  • applyAttrsToInstance still decodes entities on the JSON branch ONLY, exactly as it did at 79fc28fc. No other branch gained or lost a decode.
  • Deleting the converter arm reds at least one test at each of the unit, browser, and Bun layers.
  • test/bun/attribute-converter-parity.mjs and its .test.mjs wrapper exist, and node test/bun/attribute-converter-parity.mjs, bun test/bun/attribute-converter-parity.mjs, and node scripts/run-bun-tests.js are all reported green in the PR.
  • The browser assertion goes through a REAL element upgrade (customElements.whenDefined plus updateComplete), not a hand-called attributeChangedCallback, and npm run test:browser is green.
  • npm test is green, and webjs check is clean.
  • import('@webjsdev/core/server') still loads in a bare Node process with no DOM, and the browser bundle is unchanged (render-server.js is still absent from index-browser.js's graph).
  • All four stale "the SSR reader has no converter arm" comments are gone: component.js:1194-1197, render-server.js:1757-1762, reflect-function-guard.test.js:527-528, test/bun/reflect-unserializable.mjs:18.
  • The PropertyDeclaration typedef at component.js:86-90 no longer says fromAttribute is called in attributeChangedCallback alone.
  • Docs updated at website/app/docs/components/page.ts, .agents/skills/webjs/references/components.md, and root AGENTS.md, each naming the server-side execution and the browser-global hazard.
  • The commit subject is conventional (fix:) and its body states the server-side converter execution as a behaviour change for existing apps.

Out of scope

#1341's three reader-set divergences. Do not touch any of them. #1341 edits the SAME function, applyAttrsToInstance, and is planned against the architecture this issue establishes. Confine this diff to the shared reader plus the two call sites, so the two PRs do not race. The three cases that belong to #1341, named explicitly:

  1. A state: true prop whose attribute the SSR reader reads, which observedAttributes at packages/core/src/component.js:579 filters out on the client, so the browser never reads it.
  2. A camelCase source attribute that the SSR reader resolves through its camelCase(key) fallback at render-server.js:1716-1717, which the browser lowercases away before the client reader ever sees it.
  3. unescapeAttr at render-server.js:1799 decoding only &lt;, &quot;, and &amp;, where a browser decodes every HTML entity.

Do not widen the decode parameter, do not make it full-entity, and do not move it onto the other branches. Those are exactly #1341's calls.

Do not change attributeChangedCallback's behaviour. Its precedence is the reference implementation. Step 2 replaces its inline chain with a call to the function that chain became; the values it produces must not move. Its name resolution at L1149-1161 is untouched.

Do not touch the toAttribute / reflection path. _reflectAttribute at component.js:751 already runs one shared path for SSR and the client, and the #1169 / #1253 / #1335 guards on it are settled. This issue is the READ side only.

Do not conflate the .prop=${value} hydration channel. data-webjs-prop-* and consumePropAttrs in render-server.js are a DIFFERENT channel that already round-trips rich values through the serializer, and applyAttrsToInstance runs after it at the call site (render-server.js:973-974, where property bindings win a name collision). A converter applies to the ATTRIBUTE channel only. No change there.

Do not extend webjs check's no-browser-globals-in-render to converter bodies. It is a genuine gap: the rule at packages/server/src/check.js:677-701 scans only the constructor, willUpdate, and render method bodies of a class … extends WebComponent, and a converter is a function literal inside the factory ARGUMENT, so it is invisible to the rule; a converter defined in another module is invisible in principle. Left alone deliberately, for three reasons. The rule's own documented posture is conservative, and it already states that it does not follow helper indirection because "the runtime SSR error covers that case", which is precisely what happens here (a loud [webjs] SSR failed for <tag> plus the error box). Covering a converter needs a different extractor than methodBodyOf, which is a checker change, not a renderer change, and does not belong in the same review as an SSR reader change. And the hazard is carried instead by the three doc surfaces and the commit body, which is where an author of a converter will actually meet it.

Do not add a back-compat flag, opt-out, or deprecation path for the previous SSR behaviour. WebJs has no users yet, so the clean change is correct.

Do not file follow-up issues for anything found along the way. Fold a small same-file tweak into this PR and raise anything genuinely separate in conversation.

Metadata

Metadata

Assignees

Labels

bugSomething isn't working

Type

No type

Projects

Status
Done

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions