fix(jsx-email): align Conditional/Raw via rehype #377
Conversation
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
The biggest blocker is that both getConditionalPlugin() and getRawPlugin() mutate AST parents using indices captured before mutation, which will break when multiple matching nodes are siblings under the same parent (stale-index bug). There are also avoidable any/@ts-ignore escapes (conditional.ts property reads and JSX intrinsic element typing) that reduce safety in a security/HTML-generation path. Fixing the index mutation strategy should be required before merging to avoid silently corrupted HTML output in real templates.
Additional notes (2)
- Compatibility |
packages/jsx-email/src/renderer/raw.ts:3-3
unescapeForRawComponent() changes comment close from --> to /-->
unescapeForRawComponent() now replaces __COMMENT_END with '/-->' (note the leading slash) rather than the actual comment terminator '-->'. That means <Raw> content containing a literal '-->' will be mutated to '/-->' after a round trip.
Even if this was intended to support MSO <![endif]/-->, it’s not generally correct for arbitrary raw HTML and can corrupt user-supplied markup (especially templating content that includes comment endings).
escapeForRawComponent() and unescapeForRawComponent() should be inverse operations.
- Compatibility |
packages/jsx-email/src/renderer/render.ts:81-81
Regex removal of JSX markers is too broad and can corrupt user content
reJsxTags is used on the final HTML string to remove any stray <jsx-email-cond ...> tags. The new pattern matches tags with attributes, which helps, but it still performs a blind global string replacement.
If a user includes literal text/content containing <jsx-email-cond ...> (e.g., in a code sample, preformatted content, or templating output), this will strip it unexpectedly.
Given you now have a rehype plugin explicitly responsible for transforming/removing these markers, the string-level regex cleanup should be unnecessary (or at least limited to only truly impossible leftover cases).
Summary of changes
What this PR changes
Rendering pipeline
- Switches
<Conditional>from string-concatenated HTML (viaSuspense+jsxToString) to emitting a marker element:<jsx-email-cond data-mso data-expression data-head>. - Introduces a new rehype plugin factory
getConditionalPlugin()that:- Replaces
<jsx-email-cond>nodes withrawHAST nodes containing the appropriate conditional comment open/close wrappers. - Uses the Outlook-friendlier closer
<![endif]/-->for MSO/expression conditionals. - Optionally moves conditional content into
<head>whendata-headis set.
- Replaces
- Expands the rehype-based Raw handling by adding
getRawPlugin()to convert<jsx-email-raw><!--...--></jsx-email-raw>into arawnode early in the pipeline. - Updates
render()’sprocessHtml()to dynamically importrehype+rehype-stringify, and to apply plugins in order:movePlugin→rawPlugin→conditionalPlugin. - Broadens the post-processing tag stripper regex to remove
<jsx-email-cond>with attributes.
Components
- Updates
<Head>to inject the MSO XML block using<Conditional head mso><Raw ... /></Conditional>instead of embedding nested<head>/ JSX<xml>. - Adds a module augmentation so TypeScript accepts the custom element tag
jsx-email-cond.
Tests & docs
- Adds regression tests:
- MSO closer correctness (
<![endif]/-->). - No duplication when nesting
<Raw>inside<Conditional>. - Multiline
<Raw>content preservation.
- MSO closer correctness (
- Updates snapshots accordingly.
- Minor documentation formatting/import ordering tweaks.
| return function transform(tree: Root) { | ||
| const matches: Match[] = []; | ||
| let headEl: Element | undefined; | ||
|
|
||
| visit(tree, 'element', (node, index, parent) => { | ||
| if (node.tagName === 'head') headEl = node; | ||
|
|
||
| if (!parent || typeof index !== 'number') return; | ||
| if (node.tagName !== 'jsx-email-cond') return; | ||
|
|
||
| matches.push({ index, node, parent }); | ||
| }); | ||
|
|
||
| for (const { node, parent, index } of matches) { | ||
| const props = (node.properties || {}) as Record<string, unknown>; | ||
| const msoProp = (props['data-mso'] ?? (props as any).dataMso) as unknown; | ||
| const msoAttr = | ||
| typeof msoProp === 'undefined' ? void 0 : msoProp === 'false' ? false : Boolean(msoProp); | ||
| const exprRaw = (props['data-expression'] ?? (props as any).dataExpression) as unknown; | ||
| const exprAttr = typeof exprRaw === 'string' ? exprRaw : void 0; | ||
| const headProp = (props['data-head'] ?? (props as any).dataHead) as unknown; | ||
| const toHead = | ||
| typeof headProp === 'undefined' | ||
| ? false | ||
| : headProp === 'false' | ||
| ? false | ||
| : Boolean(headProp); | ||
|
|
||
| let openRaw: string | undefined; | ||
| let closeRaw: string | undefined; | ||
|
|
||
| if (msoAttr === false) { | ||
| // Not MSO: <!--[if !mso]><!--> ... <!--<![endif]--> | ||
| openRaw = '<!--[if !mso]><!-->'; | ||
| closeRaw = '<!--<![endif]-->'; | ||
| } else { | ||
| // MSO / expression path | ||
| const expression = exprAttr || (msoAttr === true ? 'mso' : void 0); | ||
| if (expression) { | ||
| openRaw = `<!--[if ${expression}]>`; | ||
| // Older Outlook/Word HTML parsers prefer the self-closing | ||
| // conditional terminator variant to avoid comment spillover | ||
| // when adjacent comments appear. Use the `<![endif]/-->` form | ||
| // for maximum compatibility. | ||
| closeRaw = '<![endif]/-->'; | ||
| } | ||
| } | ||
|
|
||
| // If no directive attributes present, leave the element in place. | ||
| // eslint-disable-next-line no-continue | ||
| if (!openRaw || !closeRaw) continue; | ||
|
|
||
| const before: Raw = { type: 'raw', value: openRaw }; | ||
| const after: Raw = { type: 'raw', value: closeRaw }; | ||
| const children = (node.children || []) as Content[]; | ||
|
|
||
| if (toHead && headEl) { | ||
| if (parent === headEl) { | ||
| // Replace in place: open raw, original children, close raw. | ||
| (parent as ParentWithRaw).children.splice(index, 1, before, ...children, after); | ||
| } else { | ||
| // Remove wrapper from current location | ||
| (parent as ParentWithRaw).children.splice(index, 1); | ||
| // Append the conditional to the <head> | ||
| (headEl as unknown as ParentWithRaw).children.push(before, ...children, after); | ||
| } | ||
| } else { | ||
| // Replace in place: open raw, original children, close raw. | ||
| (parent as ParentWithRaw).children.splice(index, 1, before, ...children, after); | ||
| } | ||
| } |
There was a problem hiding this comment.
getConditionalPlugin() collects matches with (index, parent) and then mutates the tree using the original indices. This is unsafe when multiple <jsx-email-cond> nodes share the same parent: after the first splice() replacement (which inserts multiple nodes), later index values for the same parent become stale and will point at the wrong node.
Example failure mode: two sibling conditionals in the same parent—after expanding the first into [before,...children,after], the second match’s stored index will no longer refer to the second conditional element.
This can lead to corrupted output, misplaced conditionals, or dropped content—especially in templates with multiple conditionals in <body> or <head>.
Suggestion
Consider transforming in a single pass without storing indices, or mutate safely by iterating per-parent in descending index order.
One robust pattern:
- During
visit, group matches byparentand collect indices. - For each
parent, sort matches byindexdescending and then splice.
Sketch:
const byParent = new Map<Parents, Match[]>();
...
const list = byParent.get(parent) ?? [];
list.push({ parent, index, node });
byParent.set(parent, list);
...
for (const [parent, list] of byParent) {
list.sort((a,b) => b.index - a.index);
for (const { node, index } of list) {
// splice safely
}
}If you’d like, reply with "@CharlieHelps yes please" and I can add a commit implementing this safely (including coverage for multiple siblings).
| for (const { node, parent, index } of matches) { | ||
| const props = (node.properties || {}) as Record<string, unknown>; | ||
| const msoProp = (props['data-mso'] ?? (props as any).dataMso) as unknown; | ||
| const msoAttr = | ||
| typeof msoProp === 'undefined' ? void 0 : msoProp === 'false' ? false : Boolean(msoProp); | ||
| const exprRaw = (props['data-expression'] ?? (props as any).dataExpression) as unknown; | ||
| const exprAttr = typeof exprRaw === 'string' ? exprRaw : void 0; | ||
| const headProp = (props['data-head'] ?? (props as any).dataHead) as unknown; | ||
| const toHead = | ||
| typeof headProp === 'undefined' | ||
| ? false | ||
| : headProp === 'false' | ||
| ? false | ||
| : Boolean(headProp); |
There was a problem hiding this comment.
getConditionalPlugin() uses (props as any).dataMso / dataExpression / dataHead. This is an unnecessary any escape that weakens the guarantees in a critical HTML-generation path.
Since you already cast node.properties to Record<string, unknown>, you can handle the alternate property casing without any by reading keys from that record.
Suggestion
Avoid any by accessing camel-cased keys via the same Record<string, unknown>:
const props = (node.properties ?? {}) as Record<string, unknown>;
const msoProp = props['data-mso'] ?? props.dataMso;
const exprRaw = props['data-expression'] ?? props.dataExpression;
const headProp = props['data-head'] ?? props.dataHead;This keeps the code type-safe without broad any casts.
Reply with "@CharlieHelps yes please" if you want me to add a commit removing the any and adjusting the parsing accordingly.
| declare module 'react/jsx-runtime' { | ||
| namespace JSX { | ||
| interface IntrinsicElements { | ||
| // @ts-ignore | ||
| 'jsx-email-cond': React.DetailedHTMLProps< | ||
| React.HTMLAttributes<HTMLElement> & { | ||
| 'data-expression'?: string; | ||
| 'data-head'?: boolean; | ||
| 'data-mso'?: boolean; | ||
| }, | ||
| HTMLElement | ||
| >; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The module augmentation for react/jsx-runtime uses // @ts-ignore and globally declares an intrinsic element. This is a fairly heavy-handed TS escape and can create friction if the package is consumed in different JSX runtimes or with different TS configs.
If you only need typing locally for this package, prefer putting the intrinsic element typing in a .d.ts file and avoid @ts-ignore, or scope it via JSX.IntrinsicElements in a types module that’s part of your build output.
Suggestion
Move the intrinsic element declaration into a dedicated *.d.ts (e.g. packages/jsx-email/src/types/jsx-email-elements.d.ts) and remove the // @ts-ignore.
Additionally, consider augmenting global JSX (or react) depending on your supported JSX runtimes.
Reply with "@CharlieHelps yes please" if you want me to add a commit that relocates this typing and eliminates the suppression.
Component / Package Name:
jsx-email
This PR contains:
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 #372
Description
Ports the
mainrehype-based pipeline for<Conditional>and<Raw>ontonext/v3.getRawPlugin()+getConditionalPlugin()(raw runs before conditional).<Conditional>now renders a marker element withdata-*attributes and lets rehype emit the final conditional comment HTML.<Head>now injects the MSO XML block via<Conditional head mso><Raw .../></Conditional>(no nested<head>).<![endif]/-->).<Raw>inside<Conditional>.<Raw>content preservation.Verification
moon run repo:lint: 0 errors (31 warnings; pre-existing).jsx-email:test: 43 files, 186 tests passed.pnpm moon run smoke:run.cibecause it runsplaywright install --with-deps, which is not reliably runnable in this devbox.Self-review notes
pnpm-workspace.yaml,shared/tsconfig.base.json, and some CLI/smoke test files that are not part of this PR’s diff; this looks like a baseline mismatch (comparingmainvsnext/v3).