fix(jsx-email): use standard MSO closer in <head> #404
Conversation
MSO conditionals rendered in <head> now close with `<![endif]-->` to match standard conditional comment syntax.
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
The core behavior change looks targeted and well-covered, but rendersInHeadScope is currently keyed to headEl && toHead, which may create surprising closer selection when head is requested but a <head> root isn’t present. The new tests rely on a regex-based <head> extraction and could be hardened to better guarantee the conditional was actually relocated into <head> (not just that the head slice lacks the slashed closer). Addressing these would make the behavior more intentional and reduce brittleness.
Additional notes (3)
- Maintainability |
packages/jsx-email/src/renderer/conditional.ts:60-60
rendersInHeadScopeis coupled to the relocation logic (as your comment notes), but usingBoolean(headEl && toHead)means the closer depends on whether a<head>root exists, not strictly whether the conditional will ultimately be rendered in<head>.
That can produce a surprising behavior split:
headprop set but noheadEl(e.g., fragment render / partial document) ⇒ you’ll emit the non-head closer even though the caller asked forhead.
If the intent is “use the standard closer whenever the component is declared as head-scoped”, keying off toHead alone is more consistent. If the intent is “only use the standard closer when we actually relocate into <head>”, then this is OK but should be guarded by a regression test that demonstrates the no-headEl case (so the behavior is deliberate and stable).
- Maintainability |
packages/jsx-email/src/renderer/conditional.ts:60-60
rendersInHeadScopeis now the pivot for whether the closer is<![endif]-->vs<![endif]/-->. As written, it’s derived fromheadEl && toHead, but the code comment says it “must stay in sync with the relocation logic”. That’s a brittle coupling: if relocation logic changes (e.g., conditionals are kept in place inside<head>even without a “head root”, or the plugin begins constructing a<head>later), the closer choice could silently diverge from where the nodes actually land.
Given this is a correctness fix for Outlook parsing, it would be safer to base the closer choice on the node’s actual location at emission time rather than on a flag that approximates relocation intent.
- Readability |
packages/jsx-email/test/conditional-endif-closer.test.tsx:6-9
The helpergetHead()parses HTML with a non-trivial regex. In this suite you only need to assert that the head-scoped conditional produced the correct closer in the head portion; but as written, if<head>appears in an attribute value or malformed HTML ever sneaks in, this could produce misleading slices.
Given the renderer is deterministic, a simpler and safer approach is to split on </head> and take the prefix, or assert relative ordering (opener before closer) within that prefix.
Summary of changes
What changed
jsx-email MSO conditional closer behavior
- Updated MSO conditional rendering to choose the closer based on head-scope:
- Inside
<head>: uses the standard closer<![endif]--> - Outside
<head>: keeps the existing Outlook-friendly closer<![endif]/-->
- Inside
- Introduced a
rendersInHeadScopeflag derived fromheadEl && toHeadto keep closer selection aligned with the existing relocation logic.
Tests & snapshots
- Added a dedicated test suite (
packages/jsx-email/test/conditional-endif-closer.test.tsx) covering:- head vs non-head closers
- expression-based conditionals
- the OfficeDocumentSettings
<xml>case
- Updated multiple snapshots to reflect the slashless closer in
<head>output.
Files touched
- Source:
packages/jsx-email/src/renderer/conditional.ts - Tests:
packages/jsx-email/test/conditional-endif-closer.test.tsx - Snapshot updates across render/debug/tailwind suites.
How can we improve that? |
|
Reviewing the new comment on PR #404 and getting ready to respond in the context of the existing changes.
|
Expand this to see my work.
|
|
Improvement I went with: treat Implemented in Notes
Verification$ pnpm install
# Build artifacts some suites rely on
$ pnpm moon run jsx-email:build
$ pnpm moon run plugin-inline:build plugin-minify:build plugin-pretty:build
# Lint (0 errors; warnings only)
$ pnpm moon run repo:lint
# Tests (Node 18.20.8)
$ PATH="$HOME/.local/share/mise/installs/node/18.20.8/bin:$PATH" node_modules/.bin/moon run jsx-email:test.ci
# Test Files 44 passed; Tests 191 passed
# TypeScript (emits into src; run from clean state)
$ git clean -fd packages/jsx-email/src
$ PATH="$HOME/.local/share/mise/installs/node/18.20.8/bin:$PATH" pnpm moon run jsx-email:tsc
# exit 0 |
|
@CharlieHelps what if a use puts a conditional within head manually but doesn't use the head or data-head properties? |
Expand this to see my work.
|
|
If someone “manually” nests Reason: So the rule of thumb is: if you want a conditional to actually land in |
|
@CharlieHelps is that documented on the website? |
Expand this to see my work.
|
|
Partially. The website does have a If you want, I can follow up with a small docs tweak in |
|
@CharlieHelps yes please update the docs |
Expand this to see my work.
|
|
Pushed a docs update to the What changed
|
|
@CharlieHelps our collaborator pushed an update which is a significant departure from the general context of this PR initially. He did some research about this and feels the slash is not needed. One of the reasons he cites is MJML not rendering any conditional comments with the slash-hack. He cites mjmlio/mjml#1284 as evidence of rendered output not containing the slash. Don't write any code, but research the following:
|
Expand this to see my work.
|
|
I dug through MJML’s code + git history and couldn’t find any instance of the 1) MJML code: any use of the slash-hack?No. In MJML core, the closer is defined as the standard
MJML’s default HTML skeleton (
And the “adjacent comments” problem is handled via merging (regex cleanup) — still using the standard closer:
2) MJML commits that added/removed the slash-hack?I couldn’t find any evidence MJML ever had it.
For additional confidence: the commit that introduced the
The “merge adjacent conditionals” helper was added in:
(That’s the closest I saw to an explicit “we had conditional-comment boundary issues, here’s how we mitigated them”.) 3) Web mentions of the slash-hack (application / discussion / validity)I searched for the literal string What is widely documented is the standard syntax (no slash):
The issue your collaborator cited (mjmlio/mjml#1284) is also not about the slash-hack — it’s about the downlevel-revealed Net: MJML is good evidence that you can ship without the slash-hack (they do), but it doesn’t provide a “they removed it because…” story — it looks like MJML never used it, and instead dealt with “conditional adjacency” via merge/cleanup. |
|
@CharlieHelps testing your beta status |
3 participants
Component / Package Name:
jsx-email
This PR contains:
Head-scoped MSO conditionals now use the standard closer (
<![endif]-->, without the slash) while keeping the existing<![endif]/-->closer for conditionals rendered outside<head>.Are tests included?
Breaking Changes?
If yes, please include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.
List any relevant issue numbers:
resolves #403
Description
This is a targeted fix for Classic Outlook’s conditional-comment parsing: MSO conditional blocks that render inside
<head>now emit the standard<![endif]-->closer.The existing
<![endif]/-->closer remains in place for non-head conditionals to preserve the behavior introduced in #330.Note: per the maintainer request on #403, I did not run tests/validation as part of opening this PR.