fix: bundle jsdom v29 CSS assets so the language server starts - #523
Conversation
|
Walkthrough
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/marko-esbuild.mts (1)
85-88: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFail fast when a patch rewrite does not apply.
These rewrites are intentionally version-specific; if upstream formatting changes,
replace(...)can silently no-op and reintroduce startup crashes. Consider asserting each expected pattern matched (or count replacements) and throw a clear build error when it doesn’t.Also applies to: 130-143
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/marko-esbuild.mts` around lines 85 - 88, The replace() call on the file contents in the marko-esbuild.mts script silently fails to match the pattern 'require.resolve("./xhr-sync-worker.js")' if upstream formatting changes, causing startup crashes without warning. After calling replace() on the file contents, verify that the replacement actually occurred by asserting that the original content contained the expected pattern or by comparing before/after strings, and throw a clear descriptive build error if the pattern was not found. Apply this same validation approach to all similar patch rewrite patterns throughout the file (also affects the similar replace operations around lines 130-143).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/marko-esbuild.mts`:
- Around line 109-112: In the replace method call within the contents assignment
(around the regex pattern for fs.readFileSync), change the second argument from
the string value JSON.stringify(css) to a function callback that returns
JSON.stringify(css). This ensures the replacement text is treated as a literal
string without interpreting special $ tokens like $&, $1, etc., preventing
potential CSS corruption if the stylesheet contains these characters.
---
Nitpick comments:
In `@scripts/marko-esbuild.mts`:
- Around line 85-88: The replace() call on the file contents in the
marko-esbuild.mts script silently fails to match the pattern
'require.resolve("./xhr-sync-worker.js")' if upstream formatting changes,
causing startup crashes without warning. After calling replace() on the file
contents, verify that the replacement actually occurred by asserting that the
original content contained the expected pattern or by comparing before/after
strings, and throw a clear descriptive build error if the pattern was not found.
Apply this same validation approach to all similar patch rewrite patterns
throughout the file (also affects the similar replace operations around lines
130-143).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9609375a-01cb-4c36-a5db-3c32fe6f0a34
📒 Files selected for processing (1)
scripts/marko-esbuild.mts
ff97393 to
01a6fd8
Compare
jsdom 29's new CSS engine breaks the esbuild-bundled language server in three ways and crashes it on startup (the VS Code extension failed to activate with "Pending response rejected since connection got disposed"). Fixed with a small patch-package + esbuild split: - jsdom locates its synchronous-XHR worker via a top-level require.resolve of an asset esbuild can't bundle. Sync XHR is unused, so a patch removes the feature outright (patches/jsdom+29.1.1.patch). - css-tree loads its JSON data via createRequire(import.meta.url), which resolves against the output bundle once bundled. A patch switches those to static JSON imports (patches/css-tree+3.2.1.patch). - jsdom reads its default UA stylesheet from a .css asset at load. That stylesheet is load-bearing (axe relies on computed styles, e.g. the marquee "element is visible" check), so esbuild inlines it at build time and now throws if the read shape changes, failing the build loudly. Patches are applied via patch-package on postinstall.
01a6fd8 to
50b38ed
Compare
jsdom 26 → 29 switched jsdom to a new CSS engine that breaks the esbuild-bundled language server in three ways, crashing it on startup so the VS Code extension failed to activate (
Pending response rejected since connection got disposed) — failing CI onmain.Fixed with a small patch-package + esbuild split:
require.resolveof an asset esbuild can't bundle. Sync XHR is unused, so a patch removes the feature outright (patches/jsdom+29.1.1.patch).createRequire(import.meta.url), which resolves against the output bundle once bundled. A patch switches those to static JSON imports (patches/css-tree+3.2.1.patch); works both bundled (esbuild) and unbundled (Node, viawith { type: "json" })..cssasset at load. It's load-bearing (axe relies ongetComputedStyle, e.g. the marquee visibility check), so esbuild inlines it at build time, and now throws if the read shape changes so a future jsdom bump fails the build loudly instead of shipping a crashing bundle.Patches apply via
patch-packageonpostinstall(a stale patch fails the install loudly on the next dep bump).Verified: the bundled server (dev + minified prod) completes LSP
initializeand runs the full jsdom + css-tree + axe path —<img>withoutaltyields the expected diagnostic, marquee yields none (matching the fixture), and the inlined stylesheet is confirmed in the bundle. The 133 unbundled@marko/language-servertests pass against the patched deps, andbuild/lint/tscare clean.