Skip to content

fix(sse): harden sse directive — loop guard, limit validation, terminal untrack, dead code#293

Open
ErickXavier wants to merge 3 commits into
mainfrom
fix/NOJS-285
Open

fix(sse): harden sse directive — loop guard, limit validation, terminal untrack, dead code#293
ErickXavier wants to merge 3 commits into
mainfrom
fix/NOJS-285

Conversation

@ErickXavier

Copy link
Copy Markdown
Collaborator

Summary

Fixes five issues in src/directives/sse.js (NOJS-285, T2 of EPIC NOJS-283):

  1. Loop element guard — SSE on a foreach/each/for element now warns and bails, matching the existing pattern in http.js via the shared _isLoopElement() helper from loops.js.

  2. sse-limit="" validation — Empty string, zero, and negative values were silently accepted due to a truthiness check on getAttribute(). Now uses hasAttribute() so parseInt("", 10) → NaN correctly triggers the validity warning. The unbounded-memory-growth warning also fires as expected.

  3. Terminal close untrack — When the server closes the connection (readyState CLOSED), the dead EventSource is now removed from the per-origin connection tracking map. Previously it stayed, causing false-positive 6-connection warnings.

  4. Dead code removal — The try/catch around _execStatement() in the onMessage handler was unreachable — _execStatement catches all errors internally and never rethrows. Removed the redundant wrapper.

  5. Bundle delta — Net +16 bytes gzip (44010 vs 43994 baseline). New guard/validation code offset by dead-code removal. No readability sacrificed.

Test evidence

  • npx jest --no-coverage __tests__/sse.test.js50 tests pass (7 new: 4 loop guard, 2 limit validation, 1 terminal untrack)
  • npm test2201/2201 pass (full suite green)
  • Coverage on sse.js: 100% statements, 100% functions, 100% lines, 92.38% branches
  • node build.js — clean build, dist/ committed

Test plan

  • Loop guard: sse + foreach/each/for warns and opens zero connections
  • sse-limit="" and sse-limit="0" now warn "not a valid positive integer"
  • Terminal CLOSED error removes EventSource from tracking (5+3 replacement stays under 6)
  • Dead catch "sse then expression error" message never appears in warns
  • Full test suite regression-free

…rminal untrack, dead code removal

1. Add _isLoopElement() guard matching http.js pattern — warn and bail
   when sse is placed on a loop element.
2. Use hasAttribute() instead of truthiness to detect sse-limit presence.
   Empty string / zero / negative values now correctly warn and trigger
   the unbounded-memory growth warning for append/prepend modes.
3. Terminal server close (readyState CLOSED) now removes the dead
   EventSource from per-origin connection tracking, preventing false-
   positive 6-connection warnings.
4. Remove unreachable try/catch around _execStatement call — the function
   handles all errors internally and never rethrows.
@ErickXavier

Copy link
Copy Markdown
Collaborator Author

QA T7 Gate — Verdict: CHANGES_REQUESTED

Full review of the 4 sse.js fixes, run in an isolated worktree at origin/fix/NOJS-285 (a31b58b).

Blockers

B1 — Loop guard is bypassed by loop clones (src/directives/sse.js:56, root cause src/directives/loops.js:19)
The new _isLoopElement guard only bails on the template element. _LOOP_ATTRS strips the HTTP verbs (get/post/...) from clones — which is why the http.js pattern actually works — but does not strip sse. With a populated list, <div foreach="item in items" sse="/api/stream"> warns once on the template and then opens one live EventSource per clone (N connections; at N>=6 it also trips the per-origin HTTP/1.1 warning the PR set out to silence). The US-0 tests (0.1–0.3) pass only because items is never populated, so zero clones are produced.
Required: add "sse" to _LOOP_ATTRS in loops.js (same precedent as the HTTP verbs) and add a populated-loop regression test (seed items with >=1 element, assert MockEventSource._instances stays empty).

B2 — sse-limit="0" contract mismatch (src/directives/sse.js:67)
docs/md/sse.md:279 documents sse-limit as a "non-negative integer" with only "non-numeric, negative" values invalid, and the v1.20.0 CHANGELOG entry says the same. This PR's <= 0 threshold now warns 'sse-limit="0" is not a valid positive integer; ignoring.' — contradicting the documented contract. Runtime behavior is unchanged (0 = uncapped, before and after); only the warning is new.
Required (pick one): (a) restore 0 as silently valid (warn only on NaN/negative), or (b) keep the warning and bind a doc correction to T4/NOJS-287: docs/md/sse.md:279 "non-negative integer" -> "positive integer", plus a note that ""/0 warn and mean uncapped. Recommendation: (b) — the warning on a no-op 0 is genuinely informative, and the CHANGELOG wording is historical and can stand.

Should-fix (non-blocking)

  • sse.js:174 — terminal-CLOSED untrack passes the mutable closure _origin rather than the origin es was tracked under. Currently safe (a closed EventSource fires no further events, and reconnects close the old es first), but it is an invariant, not a guarantee. Consider stamping the origin on the instance and routing every close path through one _disposeConnection(es) helper.
  • evaluate.js:~2200 — pre-existing: e.message in _execStatement's catch throws a TypeError if user code does throw null. Affects all 13 bare call sites equally; suggest e?.message in a follow-up. (Removing sse.js's outer try/catch was correct — it was the codebase's only outlier and _execStatement never rethrows.)

Nits

  • sse.js:67parseInt accepts trailing garbage silently ("10abc" -> limit 10, no warning) while "0x10" is warned and treated as uncapped; the warning text does not describe the actually-accepted set.
  • sse.jshasLimit is derivable as limitAttr !== null; the second DOM lookup is redundant.
  • Pre-existing: after a terminal close renders the error template, a reactive URL change reconnects correctly but never clears the stale error DOM.
  • Future refactor: the per-directive loop guard (sse.js + http.js) could be a declarative flag on registerDirective, checked once in processElement.

Verified sound

Re-request review after B1 and the B2 decision land.

…loops

Loop clones were not stripping the `sse` attribute, causing each clone to
open its own EventSource when the loop list was populated. The template-level
_isLoopElement guard only fires on the original element (empty-list case).

Add "sse" to _LOOP_ATTRS (mirrors the HTTP verb pattern) so clones never
re-fire the SSE directive. Two regression tests cover the populated-list
path for both `each` and `foreach` variants.
@ErickXavier

Copy link
Copy Markdown
Collaborator Author

QA T7 Re-Gate — Verdict: APPROVED

Focused delta re-review of 98cd415 (fix(loops): strip sse attribute from loop clones), which addresses blocker B1 from the previous gate. Run in an isolated worktree at the PR head.

B1 (loop clones each opening an EventSource) — FIXED, verified

  • Diff scope: the commit is exactly the "sse" addition to _LOOP_ATTRS in src/directives/loops.js (+2 lines, placed and commented consistently with the HTTP verb entries), the two regression tests in __tests__/sse.test.js (+32), and the rebuilt dist/. No collateral edits; src/directives/sse.js untouched.
  • Mechanism confirmed: sse (priority 1) fires on the original loop element and bails via the _isLoopElement guard; clones previously retained sse after loop-attr stripping and each opened a real connection. Stripping sse in _LOOP_ATTRS closes exactly that gap, completing the same guard+strip pair get/post already have (pattern from 7b9b286 / PR fix(loops): add HTTP guard, else ownership, page-* strip, else-if defenses #248).
  • Revert-to-red: temporarily removing "sse", from _LOOP_ATTRS makes precisely tests 0.5 and 0.6 fail (3 and 2 EventSource instances — one per loop item), confirming the regression tests genuinely exercise the populated-loop path. Restored: 52/52 green in sse.test.js.
  • Full suite: 42 suites, 2203/2203 tests pass.
  • Build: node build.js reproduces the committed dist/iife/no.js + .map byte-identically; "sse" present in the minified _LOOP_ATTRS.
  • Prior should-fix/nit items (sse.js:174 closure invariant, parseInt edge, hasLimit redundancy, etc.): unaffected — the commit does not touch sse.js or evaluate.js.

B2 (docs sse-limit="0" contract) — resolved out-of-band

Per the B2 recommendation (option b), the doc correction is tracked in the separate docs PR (NOJS-287). Not a hold on #293.

Follow-up (pre-existing, not introduced here)

query is in HTTP_METHODS with the same priority-1 _isLoopElement guard but is not listed in _LOOP_ATTRS, so foreach + query on the same element still has the per-clone leak this commit fixes for sse. Predates this PR — suggest a small follow-up issue to add "query" to _LOOP_ATTRS with a matching regression test.

PR is clear to merge from QA's side.

Add `query` to `_LOOP_ATTRS` — it was the only HTTP verb in
`HTTP_METHODS` guarded by `_isLoopElement` but missing from the strip
list, causing one fetch per clone on populated loops.

Systematic audit confirmed no other directives are missing:
- HTTP verbs (get/post/put/patch/delete): already present
- query: ADDED (was missing — the bug)
- sse: already present (98cd415)
- page-title/page-description/page-canonical/page-jsonld: already present
- computed/watch: guarded but intentionally excluded (no per-element
  side effect; clones need their own derived state)
- else-if/else: already handled (else-if in list, else via stripElse param)

Regression tests: 3 new tests in loop-hygiene.test.js verify query
on populated loops fires zero fetches and the attribute is stripped
from clones. Revert-to-red confirmed for the attr-strip test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant