fix: honour converter.fromAttribute in the SSR attribute reader

[vivek7405]

Closes #1340

Summary

A property declaring a custom converter.fromAttribute was read one way during SSR and another way the moment the element upgraded in the browser. attributeChangedCallback tried the converter first, ahead of any type-based coercion; applyAttrsToInstance dispatched on def.type alone and never called the converter at all. <my-el mode="a"> with an upper-casing converter painted a server-side and held A after upgrade.

The type-dispatch chain moves into one shared readAttributeValue in packages/core/src/attribute-reader.js that both readers call, rather than a converter arm bolted onto the second reader. The two chains had already drifted twice (this gap and the unparseable-JSON fallback in #1253), and each drift needed the same edit applied twice in sync. lit is built the same way: @lit-labs/ssr forwards its attributeChangedCallback into the element's own reader rather than keeping a copy.

Where the shared reader lives, and why not component.js

It is packages/core/src/attribute-reader.js, a module with no .d.ts overlay, rather than an export of component.js as #1340 planned. packages/core/index.d.ts:11 re-exports the component overlay with a bare export *, so a value declared in component.d.ts joins the ROOT public type surface of @webjsdev/core while index.js exports no such runtime value, and import { readAttributeValue } from '@webjsdev/core' type-checks then crashes at load. That is the phantom the #1031 guard exists to catch, and it caught it. Dropping the declaration instead does not work either, since tsc resolves the import through the overlay and fails with TS2305. Re-exporting from index.js would silence the guard by making an internal seam app-facing API, which #1340 explicitly refused. An overlay-free module keeps the seam internal and typed from its own JSDoc, the way escape.js and binding-prefixes.js already work, and it drops a module edge rather than adding one: render-server.js now reaches the reader without importing component.js at all. Full reasoning in a comment on this PR and in the module header.

Behaviour changes

A converter.fromAttribute that previously ran only in the browser now also runs during SSR. One that touches document, window, or navigator will throw server-side where it did not before, and the component renders its error state. A converter that throws is deliberately not caught by either reader, matching the rule _reflectAttribute already states for toAttribute.

Both readers hand the converter DECODED attribute text. The client's value comes out of the DOM, which already decoded it, while the SSR reader walks the raw source tag, so passing the raw text through would have made the two sides read the same attribute differently for anything carrying a quote or an ampersand. It would also have failed in the worst direction: the documented reason to write a converter is a type the built-ins cannot parse, those parse their input, and escapeAttr encodes every " we emit, so a Map or Date converter threw at SSR on markup WebJs itself produced and rendered an empty component at a 200. The decode therefore applies to both branches that PARSE their input (the converter branch and the JSON branch). The pass-through branches (String, Number, Boolean) are untouched.

Neutrality for props with no converter

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 that call site and every branch produces the value it produced before: Number('') is 0, '' !== 'false' is true, JSON.parse('') throws to null. Pinned by an exact-output test covering String, Number, Boolean, Object, and Array against a bare boolean attribute, an empty-string attribute, unparseable JSON, and an entity-encoded JSON attribute.

Merge order

Must merge BEFORE #1341, which edits the same applyAttrsToInstance. This PR is confined to converter precedence: #1341's three reader-set divergences (the state: true prop, the camelCase source attribute, and the three-entity reach of unescapeAttr) are untouched. The decode is applied only to the branches that parse, which is where the SSR reader already applied it plus the new converter branch that needs it for the same reason; whether it should reach the pass-through branches and whether three entities is enough remain #1341's calls.

Test plan

• Unit (SSR), packages/core/test/rendering/ssr-prop-options.test.js: converter runs at SSR ahead of type coercion; converter beats the declared type; converter receives decoded text for an entity-encoded attribute; a throwing converter is not caught and the component renders its SSR error state; a no-converter probe emits an exact expected string.
• Unit (client), packages/core/test/lifecycle/component-lifecycle.test.js: a throwing converter propagates out of attributeChangedCallback, pinning the other half of the no-catch decision.
• Browser, packages/core/test/rendering/browser/reflect-function-guard.test.js: two agreement tests through a REAL element upgrade (customElements.whenDefined plus updateComplete), one plain and one entity-encoded, each comparing against renderToString of the same markup. npm run test:browser green: Chromium 840, Firefox 830, Webkit 840, 0 failed.
• Bun, test/bun/attribute-converter-parity.mjs plus its .test.mjs wrapper: converter runs at SSR, beats the declared type, receives decoded text, and a throwing converter is isolated while its sibling still renders. Green under both node and bun 1.3.14.
• Counterfactuals: deleting the converter arm reds 3 SSR tests, 2 client tests, the browser agreement suite, and the Bun assertions. Reverting the decode reds the SSR decoded-text test. The no-converter neutrality test stays green through both, which is the point.
• E2E: WEBJS_E2E=1 node --test test/e2e/e2e.test.mjs green, 94/94.
• Node suite: npm test 4149/4156, with 5 failures reproduced identically on a clean origin/main worktree (2 Bun listener, 3 elision differential) and one Bun wrapper that resolves the bare @webjsdev/core specifier to the primary checkout locally; all six pass when resolution points at this branch. They are the known linked-worktree artifacts, not regressions.
• Dogfood: website boots in prod mode with 200 on /, /docs/components, /ui, /ui/button, and no broken modulepreload hints. Blog covered by the e2e run above.

Docs

• website/app/docs/components/page.ts: the converter bypasses the coercion list, runs on both readers, must avoid browser globals, throws uncaught, and receives decoded text.
• .agents/skills/webjs/references/components.md: converter row plus a paragraph carrying the same facts.
• AGENTS.md: one sentence in the property-options list.
• packages/core/src/component.js: the PropertyDeclaration typedef no longer says fromAttribute is called in attributeChangedCallback alone.
• All four source comments that stated the SSR reader has no converter arm are replaced.

N/A: the scaffold and its generators (no template or gallery demo declares a converter), the MCP server (no tool projection changes), the editor plugins (no grammar, snippet, or language-service change), marketing copy, README.md, and webjs check (no rule added or changed).

[vivek7405]

Design rationale: why the shared reader got its own module instead of living in component.js

The plan in #1340 put readAttributeValue in packages/core/src/component.js, exported, with a matching declaration in component.d.ts and deliberately no re-export from index.js, on the grounds that it is an internal seam between two framework modules rather than app-facing API. I built it that way first, and the #1031 phantom guard in test/types/dts-no-phantom-exports.test.mjs rejected it:

@webjsdev/core overlays declare value exports the runtime .js does not provide
(a type-checking import of these would crash at load):
  index.d.ts: readAttributeValue

The reason is packages/core/index.d.ts:11, which re-exports the overlay with a bare export * from './src/component.d.ts'. So anything declared as a value in component.d.ts joins the ROOT public type surface of @webjsdev/core, while index.js exports only WebComponent and prop. An app writing import { readAttributeValue } from '@webjsdev/core' would have type-checked and then crashed at load. That is a genuine phantom, not a false positive, and the guard is right to refuse it.

Three ways out, and why this one:

1. Drop the .d.ts declaration entirely. Does not work: with component.d.ts present, tsc resolves ./component.js through the overlay, so render-server.js's import fails with TS2305: Module './component.js' has no exported member 'readAttributeValue'. Confirmed by running tsc against the file directly.
2. Re-export it from index.js and index-browser.js. Silences the guard by making an internal framework seam app-facing API, and puts it in the browser bundle's public surface forever. That is the trade The SSR attribute reader ignores converter.fromAttribute #1340 explicitly refused, and I agree with the refusal.
3. Move the reader to packages/core/src/attribute-reader.js with no .d.ts overlay. The seam stays internal, it is typed from its own JSDoc, and nothing reaches index.d.ts. Seven of the 33 modules under packages/core/src/ already work this way (escape.js, binding-prefixes.js).

I took 3. It also drops a module edge instead of adding one: render-server.js now reaches the reader without importing component.js at all, so the "does this create a cycle" question the issue worked through goes away rather than being answered.

Worth noting #1340 anticipated this shape and set it aside for a reason that does not apply here. It rejected a leaf module because moving hyphenate / camelCase alongside it would be rename churn unrelated to the converter, and left the door open with "readAttributeValue can move with them at that point". Those two helpers stay exactly where they are in both files; only the reader moved. The one argument that survives is the taste one, that the reader ideally sits beside the declaration semantics it interprets, and the phantom guard outranks it.

The comparison to lit in the issue still holds, just in the other direction, and the module header says so. @lit-labs/ssr reaches INTO the element for _$attributeToProperty because lit's elements are TypeScript with a generated declaration. WebJs ships buildless JS under a hand-written overlay, so the shared code moves out instead of the server pass reaching in.

[vivek7405]

Went looking for whatever this gets wrong and found two things, both real, both now fixed.

The one that matters is the decode. The whole claim of this change is that the two readers agree on a converter, and the first cut handed the SSR converter the literal characters between the quotes while the browser hands it text the DOM already decoded. So the two sides disagreed the moment an attribute carried a quote or an ampersand, which is the exact failure the change is supposed to remove. Worse, it failed loudly in the wrong direction: the documented reason to write a converter is a type the built-ins cannot parse, those parse their input, and escapeAttr encodes every " we emit, so a Map or Date converter threw at SSR on markup we produced ourselves, and since the throw is deliberately uncaught that is an empty component at a 200 in production. Before this change the same component rendered with a wrong-but-present value. Trading a wrong value for a missing component is not a fix. Inline below.

The second was structural and is worth writing down because the plan called it the other way. Putting readAttributeValue in component.js and declaring it in component.d.ts cannot work: index.d.ts:11 re-exports that overlay with a bare export *, so the declaration joins the root public type surface of @webjsdev/core with no runtime export behind it, and import { readAttributeValue } from '@webjsdev/core' type-checks then crashes at load. That is the #1031 phantom guard's whole purpose and it caught it. Dropping the declaration does not work either, since tsc then resolves the import through the overlay and fails. So the reader moved to its own overlay-free module, packages/core/src/attribute-reader.js. The seam stays internal, and render-server.js now reaches the reader without importing component.js at all, which drops an edge instead of adding one. That finding sits path-level, since component.d.ts is no longer in the diff.

Both tests that should have caught these were missing, and both are now there at the unit, browser, and Bun layers. The converter fixtures were all entity-free, so the parity claim was asserted only in the region where it happened to hold, which is how I would expect this class of bug to survive a suite.

[vivek7405]

Second pass, scoped to the fix commits and their blast radius: both call sites, the escapeAttr / unescapeAttr round trip, the parseAttrs to seedServerAttrs to applyAttrsToInstance ordering, the two overlay guards, the package files and exports, the dev /__webjs/core/* serving path, and the Bun matrix's auto-discovery of the new wrapper.

No functional defect. The unescapeAttr replacement order is a correct inverse of escapeAttr (it undoes & last, so a doubly-encoded value cannot collapse early), and the type branches stay provably neutral because parseAttrs hands them a string for every attribute and never a null.

What it did catch is bookkeeping the module move left behind: three comments still named component.js as the reader's home, and the Bun proof's docblock enumerated three assertions where the file now runs four, so it understated its own coverage. Inline below. The same staleness had reached the PR body, which matters more than a comment because that text becomes the squash-commit body and feeds the generated changelog; fixed there too.