fix(jsx-email): align Conditional/Raw via rehype #388
Conversation
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
The biggest concern is correctness: both rehype plugins collect indices then mutate the tree, which can mis-target nodes when multiple markers share a parent (especially in getConditionalPlugin() where nodes may be removed and appended to <head>). There’s also avoidable use of any and @ts-ignore that weakens the new, more structured pipeline. Finally, the marker-stripping regex safety net should likely include jsx-email-raw now that the old regex-based Raw unescape pass is removed.
Additional notes (1)
- Compatibility |
packages/jsx-email/src/renderer/render.ts:12-12
This new regex removesjsx-emailmarker tags with attributes, which is good. However,jsxEmailTagscurrently only includesjsx-email-cond, whilejsx-email-rawalso exists in the intermediate HTML (especially ifgetRawPlugin()misses a node due to index-shift issues or unexpected structure).
If a <jsx-email-raw> wrapper survives the rehype pipeline for any reason, it will now remain in output because the old regex-based unescape post-pass was removed.
Summary of changes
What changed
Conditional rendering pipeline
- Replaced runtime conditional-comment generation (via
Suspense,useData(), anddangerouslySetInnerHTML) with a marker element approach:<jsx-email-cond data-mso data-expression data-head>…</jsx-email-cond>. - Added a local
react/jsx-runtimeaugmentation so TypeScript understands the custom intrinsic element.
Rehype processing
- Added
packages/jsx-email/src/renderer/conditional.tsimplementinggetConditionalPlugin()to convert<jsx-email-cond>markers into conditional comment wrappers. - Extended
packages/jsx-email/src/renderer/raw.tswithgetRawPlugin()to convert<jsx-email-raw>wrappers into HASTrawnodes (instead of doing a string-regex post-pass). - Updated
processHtml()inrender.tsto:- Dynamically import
rehype+rehype-stringify - Run plugins in order:
movePlugin→rawPlugin→conditionalPlugin - Remove marker tags via a broader regex (
<jsx-email-cond ...>as well as bare tags)
- Dynamically import
Head / MSO XML emission
- Updated
<Head>to emit the MSO<xml>…</xml>block via<Conditional head mso><Raw … /></Conditional>.
Tests & snapshots
- Added new regression tests for:
- MSO closer variant (
<![endif]/-->) - Avoiding Raw duplication when nested
- Raw-in-Conditional behavior across
srcanddist
- MSO closer variant (
- Updated many snapshots to reflect the new pipeline and closer behavior.
| 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 (index, node, parent) matches, then mutates parents using splice(index, …) in a forward loop. This is fragile when multiple <jsx-email-cond> nodes share the same parent: removing/replacing an earlier sibling shifts indices of later matches, so later splice() calls can target the wrong node (or skip/duplicate content). The risk increases because you also sometimes remove the node from its original parent and append into <head>.
This is a correctness issue: an email with multiple conditionals in the same container could render incorrectly depending on traversal order.
Suggestion
Fix by applying mutations in reverse document order per parent (or by using stable references rather than stored indices). One simple approach:
- Collect matches.
- Sort by
parentidentity andindexdescending, then splice.
Example:
// after collecting matches
matches.sort((a, b) => {
if (a.parent === b.parent) return b.index - a.index;
return 0; // keep relative order across different parents
});
for (const m of matches) {
// ... existing logic
}Or, even safer: when moving to head, avoid relying on the stored index by removing via identity search:
const idx = (parent as ParentWithRaw).children.indexOf(node as any);
if (idx !== -1) (parent as ParentWithRaw).children.splice(idx, 1);Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| 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.
There are multiple any casts here ((props as any).dataMso, etc.). This undermines the otherwise nice effort to keep HAST nodes typed and makes the attribute decoding harder to reason about.
Since properties is already Record<string, unknown>-like, you can avoid any by accessing camelCase keys through the same record and centralizing the coercion logic.
Suggestion
Introduce small helpers to normalize properties access without any, and share boolean parsing between data-mso and data-head.
const getProp = (p: Record<string, unknown>, dash: string, camel: string) =>
(p[dash] ?? p[camel]) as unknown;
const parseBoolAttr = (v: unknown): boolean | undefined => {
if (typeof v === 'undefined') return undefined;
if (v === 'false') return false;
return Boolean(v);
};
const msoAttr = parseBoolAttr(getProp(props, 'data-mso', 'dataMso'));
const toHead = parseBoolAttr(getProp(props, 'data-head', 'dataHead')) ?? false;
const exprRaw = getProp(props, 'data-expression', 'dataExpression');Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| 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 custom JSX intrinsic element typing relies on // @ts-ignore inside the module augmentation. That effectively disables type checking right at the point you’re trying to make it safer, and it’s easy for that to spread.
Given this is a library component, it’s worth making the augmentation clean so consumers (and this repo) don’t normalize @ts-ignore as acceptable.
Suggestion
Remove the // @ts-ignore and make the intrinsic element definition stand on its own.
In most setups, this works without suppression:
declare module 'react/jsx-runtime' {
namespace JSX {
interface IntrinsicElements {
'jsx-email-cond': React.DetailedHTMLProps<
React.HTMLAttributes<HTMLElement> & {
'data-expression'?: string;
'data-head'?: boolean;
'data-mso'?: boolean;
},
HTMLElement
>;
}
}
}If TS still complains due to module resolution differences, prefer moving this augmentation into a dedicated *.d.ts (e.g. src/jsx.d.ts) that’s included by tsconfig, instead of suppressing errors inline.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| import { Conditional, Raw, render } from '../dist/index.js'; | ||
| import { jsxToString } from '../src/renderer/jsx-to-string.js'; |
There was a problem hiding this comment.
This test mixes ../dist/index.js and ../src/... imports in the same file. That creates a real risk of testing a “hybrid” of built output and source implementation (and can accidentally pull in different React instances or different module formats).
You already note hermetic tests elsewhere; this file does the opposite. It also makes the test suite sensitive to build output layout and bundler behavior.
Suggestion
Make the test fully hermetic by importing everything from ../src (preferred) or everything from ../dist (if you explicitly want an integration test), but don’t mix.
For example:
import { Conditional, Raw } from '../src/index.js';
import { render } from '../src/renderer/render.js';
import { jsxToString } from '../src/renderer/jsx-to-string.js';Reply with "@CharlieHelps yes please" if you'd like me to add a commit updating the imports.
| describe('Raw in Conditional', async () => { | ||
| it('Raw in Conditional', async () => { | ||
| const htmlRes = await jsxToString(<Template />); | ||
| expect(htmlRes).toMatchSnapshot(); | ||
|
|
||
| const renderRes = await render(<Template />); | ||
| expect(renderRes).toMatchSnapshot(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
This describe callback is marked async, which Vitest doesn’t await. If any setup ever depends on awaiting inside describe, it will silently not behave as expected.
Even if it currently “works”, it’s misleading and makes future edits error-prone.
Suggestion
Remove the async from describe:
describe('Raw in Conditional', () => {
it('Raw in Conditional', async () => {
// ...
});
});Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this change.
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 #380
Description
Ports
main’s marker-element + rehype-plugin pipeline for<Conditional>and<Raw>ontonext/v3.<Conditional>now renders ajsx-email-condmarker element withdata-mso/data-expression/data-headinstead of emitting conditional comments via Suspense +dangerouslySetInnerHTML.getConditionalPlugin()to turnjsx-email-condmarkers into conditional comment wrappers, always using the Outlook-friendly closer<![endif]/-->.getRawPlugin()and updated the render pipeline so Raw hoisting runs before conditional processing (avoids Raw duplication when nesting<Raw>inside<Conditional>).<Head>to matchmainby emitting the MSO<xml>…</xml>block via<Conditional head mso><Raw … /></Conditional>.conditional-endif-closer.test.tsxconditional-raw-nodup.test.tsxconditional-raw.test.tsx(adjusted for v3 dist layout)Verification
Self-review notes
pnpm-workspace.yaml,shared/tsconfig.base.json,shared/tsconfig.eslint.json, and a few test/smoke-related files, but those are pre-existingnext/v3differences vsmainand are not touched by this PR.