diff --git a/agent-feedback/items/2026-08-20-jsdom-url-so-failures-print-a-diff.md b/agent-feedback/items/2026-08-20-jsdom-url-so-failures-print-a-diff.md new file mode 100644 index 0000000..765fa56 --- /dev/null +++ b/agent-feedback/items/2026-08-20-jsdom-url-so-failures-print-a-diff.md @@ -0,0 +1,12 @@ +--- +type: bug +impact: high +effort: low +site: src/index.ts › render +--- + +# Give the server render's JSDOM a real URL so a failing assertion prints its diff + +`render()` builds its document with `new JSDOM()`, whose URL defaults to `about:blank`, an opaque origin. Any failing `expect()` in a node-environment test whose expected or received value contains a rendered node makes the runner serialize that node for the diff; the walk reaches `ownerDocument.defaultView.localStorage`, which throws `SecurityError: localStorage is not available for opaque origins`, and that error replaces the entire report — no expected/received, no code frame, no file and line, and several failures in one file collapse into a single `SecurityError` line. Every server-side assertion about the DOM is affected (`toBe`, `toEqual`, `toHaveLength` all reproduce), so the one moment a test suite is supposed to explain itself is the moment it says nothing, and finding a wrong number costs a rewrite-and-rerun loop. `new JSDOM("", { url: "http://localhost" })` restores the ordinary diff and leaves the existing suite green. + +Check: add a case to `src/__tests__/render.server.test.ts` that renders `fixtures/counter.marko` and asserts `expect(getByText("Value: 0")).toBe(null)`; today the run prints only `SecurityError: localStorage is not available for opaque origins`, and it should print the `- Expected null` / `+ Received
` diff with the failing line. diff --git a/agent-feedback/items/2026-08-20-order-independent-cleanup-opt-out.md b/agent-feedback/items/2026-08-20-order-independent-cleanup-opt-out.md new file mode 100644 index 0000000..721196c --- /dev/null +++ b/agent-feedback/items/2026-08-20-order-independent-cleanup-opt-out.md @@ -0,0 +1,12 @@ +--- +type: bug +impact: med +effort: low +site: src/index-browser.ts › cleanup +--- + +# Read `___disable_marko_test_auto_cleanup___` when cleanup runs, not when the module is evaluated + +`dont-cleanup-after-each/index.js` only sets `globalThis.___disable_marko_test_auto_cleanup___`, and the trailing block of both `src/index-browser.ts` and `src/index.ts` reads that flag once, at module evaluation, to decide whether to register `afterEach(cleanup)`. ES modules evaluate in source order, so the opt-out works only when its import is written above `@marko/testing-library` — and the natural order, `import { render } from "@marko/testing-library"` first, silently does nothing: components are still removed between tests and no message says why. The README shows the import on its own, so nothing warns the reader that position is load-bearing, and an import sorter can flip the behaviour of an untouched test file. Register the hook unconditionally and read the flag inside the callback, so it can arrive any time before the first test, or throw when it is set after registration. + +Check: two `*.browser.test.ts` files that each render `fixtures/hello-world.marko` in the first test and count `document.body.querySelectorAll("div")` in the second, one importing `../../dont-cleanup-after-each/index.js` above `../index-browser` and one below; today the second test logs 2 divs in the first file and 0 in the second with no message either way, and both should log 2. diff --git a/agent-feedback/items/2026-08-20-parse-a-document-level-template-as-a-document.md b/agent-feedback/items/2026-08-20-parse-a-document-level-template-as-a-document.md new file mode 100644 index 0000000..e080d46 --- /dev/null +++ b/agent-feedback/items/2026-08-20-parse-a-document-level-template-as-a-document.md @@ -0,0 +1,12 @@ +--- +type: bug +impact: med +effort: med +site: src/index.ts › render +--- + +# Parse a document-level template as a document instead of a fragment + +The server `render()` builds its container with `JSDOM.fragment(html)`, and a fragment parse drops ``, `` and `` and splices their children in at the top level. A template that is the document — a `@marko/run` `+layout.marko` is one, and the starter ships a test file next to it — therefore renders into a container where `querySelector("html")`, `("head")` and `("body")` are all null, so `lang`, `charset`, `viewport` and every other head assertion is unreachable; meanwhile `` text joins the body text every text query reads, so `container.textContent` interleaves head and body and `getByText` matches the title. Nothing warns that the wrapper elements were discarded. Either parse as a document when the markup contains a document element, or add a documented flag to `RenderOptions` in `src/shared.ts`; the browser build mounts into a real container and is unaffected. + +Check: add a `fixtures/` template of `<html lang="en"><head><meta charset="utf-8"><title>Roster

HELLO FROM BODY

` and render it in `src/__tests__/render.server.test.ts`; today `container.childNodes` is `META, TITLE, P`, `querySelector("html")` is null and `container.textContent` is `"RosterHELLO FROM BODY"`, and `` should be assertable with the title text out of the body text. diff --git a/agent-feedback/items/2026-08-20-reclaim-the-container-when-a-render-throws.md b/agent-feedback/items/2026-08-20-reclaim-the-container-when-a-render-throws.md new file mode 100644 index 0000000..78682ce --- /dev/null +++ b/agent-feedback/items/2026-08-20-reclaim-the-container-when-a-render-throws.md @@ -0,0 +1,12 @@ +--- +type: bug +impact: high +effort: low +site: src/index-browser.ts › render +--- + +# Reclaim the default container when a `render()` throws + +The default container is created and appended to `document.body` in the destructuring default of `options`, but the `MountedComponent` record is only added to `mountedComponents` after the mount (`template.mount()` on Marko 6, `renderResult.appendTo()` on 3-5) returns. A render that throws therefore rejects with its container already in the document and no record, so neither `cleanup()` nor the automatic `afterEach(cleanup)` — both of which iterate `mountedComponents` — can reclaim it, and the markup stays in `document.body` for the rest of the file. The next, unrelated test still resolves `screen.queryByText(...)` against it, which is the most expensive shape a test-infrastructure bug takes: the failure appears somewhere the reader has no reason to suspect, and error-path tests are exactly where it bites. Add the record before mounting, or wrap the mount so a throw removes a default container and rethrows. + +Check: add a `fixtures/` component whose `onMount` throws and a `src/__tests__/render.browser.test.ts` pair where test A does `try { await render(Thrower) } catch {}` and test B asserts `document.body.innerHTML` is empty; today B sees `
LEAKED TEXT
` and a truthy `screen.queryByText("LEAKED TEXT")`, and it should see an empty body.