-
Notifications
You must be signed in to change notification settings - Fork 53
fix(jsx-email): align Conditional/Raw via rehype #388
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: next/v3
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,106 @@ | ||
| import type { Content, Element, Literal, Parents, Root } from 'hast'; | ||
|
|
||
| // dynamic import of 'unist-util-visit' within factory to support CJS build | ||
|
|
||
| interface Match { | ||
| index: number; | ||
| node: Element; | ||
| parent: Parents; | ||
| } | ||
|
|
||
| // `raw` is an unofficial HAST node used by rehype to pass through HTML verbatim. | ||
| // Model it locally to avoid `any` casts while keeping the rest of the tree typed. | ||
| interface Raw extends Literal { | ||
| type: 'raw'; | ||
| value: string; | ||
| } | ||
|
|
||
| interface ParentWithRaw { | ||
| children: (Content | Raw)[]; | ||
| } | ||
|
|
||
| /** | ||
| * Returns a rehype plugin that replaces `<jsx-email-cond>` elements (from | ||
| * the Conditional component) with conditional comment wrappers, based on the | ||
| * `data-mso` and `data-expression` attributes. | ||
| * | ||
| * Mirrors the async factory pattern used by `getRawPlugin()`. | ||
| */ | ||
| export const getConditionalPlugin = async () => { | ||
| const { visit } = await import('unist-util-visit'); | ||
|
|
||
| return function conditionalPlugin() { | ||
| 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); | ||
|
Comment on lines
+47
to
+59
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are multiple Since SuggestionIntroduce small helpers to normalize 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. |
||
|
|
||
| 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); | ||
| } | ||
| } | ||
|
Comment on lines
+34
to
+103
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This is a correctness issue: an email with multiple conditionals in the same container could render incorrectly depending on traversal order. SuggestionFix by applying mutations in reverse document order per parent (or by using stable references rather than stored indices). One simple approach:
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 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. |
||
| }; | ||
| }; | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| // Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html | ||
|
|
||
| exports[`Raw in Conditional > Raw in Conditional 1`] = `"<jsx-email-cond data-mso="true" data-head="true"><jsx-email-raw><!--<xml><o:OfficeDocumentSettings><o:AllowPNG /><o:PixelsPerInch>96</o:PixelsPerInch></o:OfficeDocumentSettings></xml>--></jsx-email-raw></jsx-email-cond>"`; | ||
|
|
||
| exports[`Raw in Conditional > Raw in Conditional 2`] = `"<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd"><html><head><!--[if mso]><xml><o:OfficeDocumentSettings><o:AllowPNG /><o:PixelsPerInch>96</o:PixelsPerInch></o:OfficeDocumentSettings></xml><![endif]/--></head><body></body></html>"`; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,9 @@ | ||
| // Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html | ||
|
|
||
| exports[`<Head> component > renders correctly 1`] = `"<head><meta http-equiv="Content-Type" content="text/html; charset=UTF-8"/><meta name="viewport" content="width=device-width, initial-scale=1, user-scalable=yes"/><meta name="x-apple-disable-message-reformatting"/><meta name="format-detection" content="telephone=no, date=no, address=no, email=no, url=no"/><meta name="viewport" content="width=device-width, initial-scale=1, user-scalable=yes"/><meta name="x-apple-disable-message-reformatting"/><meta name="format-detection" content="telephone=no, date=no, address=no, email=no, url=no"/><head><!--[if mso]><xml><o:OfficeDocumentSettings><o:AllowPNG></o:AllowPNG><o:PixelsPerInch>96</o:PixelsPerInch></o:OfficeDocumentSettings></xml><![endif]--></head></head>"`; | ||
| exports[`<Head> component > renders correctly 1`] = `"<head><meta http-equiv="Content-Type" content="text/html; charset=UTF-8"/><meta name="viewport" content="width=device-width, initial-scale=1, user-scalable=yes"/><meta name="x-apple-disable-message-reformatting"/><meta name="format-detection" content="telephone=no, date=no, address=no, email=no, url=no"/><meta name="viewport" content="width=device-width, initial-scale=1, user-scalable=yes"/><meta name="x-apple-disable-message-reformatting"/><meta name="format-detection" content="telephone=no, date=no, address=no, email=no, url=no"/><jsx-email-cond data-mso="true" data-head="true"><jsx-email-raw><!--<xml><o:OfficeDocumentSettings><o:AllowPNG /><o:PixelsPerInch>96</o:PixelsPerInch></o:OfficeDocumentSettings></xml>--></jsx-email-raw></jsx-email-cond></head>"`; | ||
|
|
||
| exports[`<Head> component > renders style tags 1`] = ` | ||
| "<head><meta http-equiv="Content-Type" content="text/html; charset=UTF-8"/><meta name="viewport" content="width=device-width, initial-scale=1, user-scalable=yes"/><meta name="x-apple-disable-message-reformatting"/><meta name="format-detection" content="telephone=no, date=no, address=no, email=no, url=no"/><meta name="viewport" content="width=device-width, initial-scale=1, user-scalable=yes"/><meta name="x-apple-disable-message-reformatting"/><meta name="format-detection" content="telephone=no, date=no, address=no, email=no, url=no"/><style>body{ | ||
| color: red; | ||
| }</style><head><!--[if mso]><xml><o:OfficeDocumentSettings><o:AllowPNG></o:AllowPNG><o:PixelsPerInch>96</o:PixelsPerInch></o:OfficeDocumentSettings></xml><![endif]--></head></head>" | ||
| }</style><jsx-email-cond data-mso="true" data-head="true"><jsx-email-raw><!--<xml><o:OfficeDocumentSettings><o:AllowPNG /><o:PixelsPerInch>96</o:PixelsPerInch></o:OfficeDocumentSettings></xml>--></jsx-email-raw></jsx-email-cond></head>" | ||
| `; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The custom JSX intrinsic element typing relies on
// @ts-ignoreinside 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-ignoreas acceptable.Suggestion
Remove the
// @ts-ignoreand make the intrinsic element definition stand on its own.In most setups, this works without suppression:
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 bytsconfig, instead of suppressing errors inline.Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.