fix(jsx-email): align Conditional/Raw via rehype #377
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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
+46
to
+59
Contributor
Author
There was a problem hiding this comment.
Since you already cast SuggestionAvoid 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 Reply with "@CharlieHelps yes please" if you want me to add a commit removing the |
||
|
|
||
| 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
+33
to
+103
Contributor
Author
There was a problem hiding this comment.
Example failure mode: two sibling conditionals in the same parent—after expanding the first into This can lead to corrupted output, misplaced conditionals, or dropped content—especially in templates with multiple conditionals in SuggestionConsider transforming in a single pass without storing indices, or mutate safely by iterating per-parent in descending index order. One robust pattern:
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). |
||
| }; | ||
| }; | ||
| }; | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 module augmentation for
react/jsx-runtimeuses// @ts-ignoreand 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.tsfile and avoid@ts-ignore, or scope it viaJSX.IntrinsicElementsin 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(orreact) 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.