Skip to content

fix(jsx-email): fix email check + --use-preview-props #389

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.

Sign up for GitHub

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

Open
charliecreates wants to merge 2 commits into
base: next/v3
Choose a base branch
from
Open

fix(jsx-email): fix email check + --use-preview-props #389

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5ab4944
fix(jsx-email,test-cli): add --use-preview-props to email check
CharlieHelps Dec 10, 2025
c43c858
test(jsx-email): stabilize config snapshots
CharlieHelps Dec 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 20 additions & 7 deletions packages/jsx-email/src/cli/commands/check.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { type IssueGroup, caniemail, groupIssues, sortIssues } from 'caniemail';
import chalk from 'chalk';
import chalkTmpl from 'chalk-template';
import stripAnsi from 'strip-ansi';
import { type InferOutput as Infer, parse as assert, object } from 'valibot';
import { type InferOutput as Infer, parse as assert, boolean, object, optional } from 'valibot';

import { formatBytes, gmailByteLimit, gmailBytesSafe } from '../helpers.js';

Expand All @@ -13,7 +13,9 @@ import { type CommandFn } from './types.js';

const { error, log } = console;

const CheckOptionsStruct = object({});
const CheckOptionsStruct = object({
usePreviewProps: optional(boolean())
});
Comment on lines +16 to +18
Copy link
Contributor Author

@charliecreates charliecreates bot Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

usePreviewProps is defined as optional(boolean()), which means argv.usePreviewProps can be undefined. Passing undefined through to buildTemplates() can create ambiguous behavior if buildTemplates distinguishes between false and undefined (e.g., defaulting to true when missing, or changing cache keys). Since this is a CLI flag, it’s usually better to normalize to a concrete boolean (false by default) at the command boundary.

Suggestion

Normalize the flag to a strict boolean so downstream behavior is deterministic.

const CheckOptionsStruct = object({
  usePreviewProps: optional(boolean(), false)
});

If valibot doesn’t support defaults in your version, do it at the callsite:

usePreviewProps: argv.usePreviewProps ?? false,

Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.


type CheckOptions = Infer<typeof CheckOptionsStruct>;

Expand All @@ -32,17 +34,24 @@ export const help = chalkTmpl`
Check jsx-email templates for client compatibility

{underline Usage}
$ email check <template file name>
$ email check <template file name> [...options]

{underline Options}
--use-preview-props
When set, use the \`previewProps\` exported by the template file (if present).

{underline Examples}
$ email check ./emails/Batman.tsx
$ email check ./emails/Batman.tsx --use-preview-props
`;

const formatNotes = (notes: string[], indent: string) => {
if (!notes.length) return '';
const noteLines = (notes as string[]).join(`\n${'.'.repeat(indent.length)}**`);
console.log({ noteLines });
return chalkTmpl`\n${indent}{cyan Notes}:\n${'.'.repeat(indent.length)}asshole\n`;

const noteIndent = `${indent} `;
const noteLines = notes.map((note) => `${noteIndent}${chalk.dim(note)}`).join('\n');

Comment on lines 48 to +53
Copy link
Contributor Author

@charliecreates charliecreates bot Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

formatIssue() dereferences position with non-null assertions (position!.start...). If caniemail ever yields an issue without position, email check will crash while formatting output (i.e., during reporting, not during parsing). Given this is a CLI command, it’s better to degrade gracefully (e.g., show ?:?) than throw.

Also, formatNotes() now returns a plain string while most other formatting uses chalkTmpl; this is fine, but consider keeping the formatting approach consistent so future changes don’t accidentally double-escape/strip ANSI.

Suggestion

Guard the position access and format a fallback location when it’s missing. For example:

const loc = position
  ? chalkTmpl`{dim ${position.start.line}:${position.start.column}}`
  : chalk.dim('?:?');

and use loc in the formatted line instead of position!.….

Reply with "@CharlieHelps yes please" if you’d like me to add a commit with this change.

return `\n${indent}${chalk.cyan('Notes')}:\n${noteLines}`;
};

const formatIssue = (group: IssueGroup): string => {
Expand Down Expand Up @@ -129,7 +138,11 @@ export const command: CommandFn = async (argv: CheckOptions, input) => {
log(chalkTmpl`{blue Checking email template for Client Compatibility...}\n`);

const [file] = await buildTemplates({
buildOptions: { showStats: false, writeToFile: false },
buildOptions: {
showStats: false,
usePreviewProps: argv.usePreviewProps,
writeToFile: false
},
targetPath: input[0]
});

Expand Down
36 changes: 34 additions & 2 deletions packages/jsx-email/test/config/define-config.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,40 @@
import { defineConfig } from '../../src/config.js';
import { pluginSymbol } from '../../src/plugins.js';
import { createRequire } from 'node:module';
import { dirname } from 'node:path';

import loglevelnext from 'loglevelnext';

const require = createRequire(import.meta.url);

describe('defineConfig', async () => {
beforeEach(() => {
vi.restoreAllMocks();
vi.resetModules();

process.env.FORCE_COLOR = '1';

const dotLogPath = require.resolve('@dot/log');
const chalkPath = require.resolve('chalk', { paths: [dirname(dotLogPath)] });

delete require.cache[dotLogPath];
delete require.cache[chalkPath];
require(chalkPath).level = 1;

delete (globalThis as any)[Symbol.for('jsx-email/global/config')];
delete process.env.DOT_LOG_LEVEL;

for (const key of Object.keys(loglevelnext.loggers)) {
if (key === 'default') continue;
delete loglevelnext.loggers[key];
}
});

test('defaults', async () => {
const { defineConfig } = await import('../../src/config.js');
expect(await defineConfig({})).toMatchSnapshot();
});

test('basic set', async () => {
const { defineConfig } = await import('../../src/config.js');
const config = await defineConfig({
render: {
minify: true
Expand All @@ -16,6 +44,7 @@ describe('defineConfig', async () => {
});

test('minify and pretty', async () => {
const { defineConfig } = await import('../../src/config.js');
const config = await defineConfig({
render: {
minify: true,
Expand All @@ -26,6 +55,7 @@ describe('defineConfig', async () => {
});

test('plain', async () => {
const { defineConfig } = await import('../../src/config.js');
const config = await defineConfig({
render: {
minify: true,
Expand All @@ -37,6 +67,8 @@ describe('defineConfig', async () => {
});

test('de-dupe plugins', async () => {
const { defineConfig } = await import('../../src/config.js');
const { pluginSymbol } = await import('../../src/plugins.js');
const plugin = { name: 'batman', symbol: pluginSymbol };
const config = await defineConfig({
plugins: [plugin as any, plugin]
Expand Down
30 changes: 29 additions & 1 deletion packages/jsx-email/test/config/load/dotdir/load-dotdir.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,35 @@
import { loadConfig } from '../../../../src/config.js';
import { createRequire } from 'node:module';
import { dirname } from 'node:path';

import loglevelnext from 'loglevelnext';

const require = createRequire(import.meta.url);

describe('loadConfig → dotdir', async () => {
beforeEach(() => {
vi.restoreAllMocks();
vi.resetModules();

process.env.FORCE_COLOR = '1';

const dotLogPath = require.resolve('@dot/log');
const chalkPath = require.resolve('chalk', { paths: [dirname(dotLogPath)] });

delete require.cache[dotLogPath];
delete require.cache[chalkPath];
require(chalkPath).level = 1;

delete (globalThis as any)[Symbol.for('jsx-email/global/config')];
delete process.env.DOT_LOG_LEVEL;

for (const key of Object.keys(loglevelnext.loggers)) {
if (key === 'default') continue;
delete loglevelnext.loggers[key];
}
});

test('loadConfig', async () => {
const { loadConfig } = await import('../../../../src/config.js');
const config = await loadConfig(__dirname);
expect(config).toMatchSnapshot();
});
Expand Down
30 changes: 29 additions & 1 deletion packages/jsx-email/test/config/load/mjs-json/load-mjs-json.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,35 @@
import { loadConfig } from '../../../../src/config.js';
import { createRequire } from 'node:module';
import { dirname } from 'node:path';

import loglevelnext from 'loglevelnext';

const require = createRequire(import.meta.url);

describe('loadConfig → mjs-json', async () => {
beforeEach(() => {
vi.restoreAllMocks();
vi.resetModules();

process.env.FORCE_COLOR = '1';

const dotLogPath = require.resolve('@dot/log');
const chalkPath = require.resolve('chalk', { paths: [dirname(dotLogPath)] });

delete require.cache[dotLogPath];
delete require.cache[chalkPath];
require(chalkPath).level = 1;

delete (globalThis as any)[Symbol.for('jsx-email/global/config')];
delete process.env.DOT_LOG_LEVEL;

for (const key of Object.keys(loglevelnext.loggers)) {
if (key === 'default') continue;
delete loglevelnext.loggers[key];
}
});

test('loadConfig', async () => {
const { loadConfig } = await import('../../../../src/config.js');
const config = await loadConfig(__dirname);
expect(config).toMatchSnapshot();
});
Expand Down
30 changes: 29 additions & 1 deletion packages/jsx-email/test/config/load/mjs/load-mjs.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,35 @@
import { loadConfig } from '../../../../src/config.js';
import { createRequire } from 'node:module';
import { dirname } from 'node:path';

import loglevelnext from 'loglevelnext';

const require = createRequire(import.meta.url);

describe('loadConfig → mjs', async () => {
beforeEach(() => {
vi.restoreAllMocks();
vi.resetModules();

process.env.FORCE_COLOR = '1';

const dotLogPath = require.resolve('@dot/log');
const chalkPath = require.resolve('chalk', { paths: [dirname(dotLogPath)] });

delete require.cache[dotLogPath];
delete require.cache[chalkPath];
require(chalkPath).level = 1;

delete (globalThis as any)[Symbol.for('jsx-email/global/config')];
delete process.env.DOT_LOG_LEVEL;

for (const key of Object.keys(loglevelnext.loggers)) {
if (key === 'default') continue;
delete loglevelnext.loggers[key];
}
});
Comment on lines +9 to +29
Copy link
Contributor Author

@charliecreates charliecreates bot Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There’s substantial repeated beforeEach setup across multiple config tests to reset chalk/loggers/global config. This repetition makes it easy for suites to drift (one file changes, others don’t) and is hard to maintain. Centralizing this into a shared helper (or a single setup module imported by these tests) would keep the stabilization logic consistent and reduce copy/paste risk.

Suggestion

Extract the repeated stabilization logic into a shared helper used by all config tests.

Example:

// test/helpers/reset-config-test-env.ts
export function resetConfigTestEnv() {
  vi.restoreAllMocks();
  vi.resetModules();
  process.env.FORCE_COLOR = '1';
  // …chalk/@dot/log cache reset…
  // …global config + loglevelnext loggers reset…
}

Then in each test file:

import { resetConfigTestEnv } from '../helpers/reset-config-test-env';
beforeEach(resetConfigTestEnv);

Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.


test('loadConfig', async () => {
const { loadConfig } = await import('../../../../src/config.js');
const config = await loadConfig(__dirname);
expect(config).toMatchSnapshot();
});
Expand Down
30 changes: 29 additions & 1 deletion packages/jsx-email/test/config/load/parent-dir/child-dir/parent-dir.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,35 @@
import { loadConfig } from '../../../../../src/config.js';
import { createRequire } from 'node:module';
import { dirname } from 'node:path';

import loglevelnext from 'loglevelnext';

const require = createRequire(import.meta.url);

describe('loadConfig → parent dir', async () => {
beforeEach(() => {
vi.restoreAllMocks();
vi.resetModules();

process.env.FORCE_COLOR = '1';

const dotLogPath = require.resolve('@dot/log');
const chalkPath = require.resolve('chalk', { paths: [dirname(dotLogPath)] });

delete require.cache[dotLogPath];
delete require.cache[chalkPath];
require(chalkPath).level = 1;

delete (globalThis as any)[Symbol.for('jsx-email/global/config')];
delete process.env.DOT_LOG_LEVEL;

for (const key of Object.keys(loglevelnext.loggers)) {
if (key === 'default') continue;
delete loglevelnext.loggers[key];
}
});

test('loadConfig', async () => {
const { loadConfig } = await import('../../../../../src/config.js');
const config = await loadConfig(__dirname);
expect(config).toBeDefined();
expect(config).toMatchSnapshot();
Expand Down
31 changes: 31 additions & 0 deletions test/cli/check.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import { resolve } from 'node:path';

import { execa } from 'execa';
import strip from 'strip-ansi';
import { describe, expect, test } from 'vitest';

process.chdir(__dirname);

describe('cli', async () => {
test('email check supports --use-preview-props', async () => {
const templatePath = resolve(__dirname, './fixtures/check-preview-props.tsx');

await expect(
execa({ cwd: __dirname, shell: true })`email check ${templatePath}`
).rejects.toMatchObject({
exitCode: 1
});

const { stdout } = await execa({
cwd: __dirname,
shell: true
})`email check ${templatePath} --use-preview-props`;
Comment on lines +13 to +22
Copy link
Contributor Author

@charliecreates charliecreates bot Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new test uses shell: true with a template path interpolated into the command. Even though this is “just tests”, it introduces needless platform quirks (quoting/escaping spaces on Windows) and a command-injection footgun if paths ever come from variables that may include special characters. execa supports passing args without shell—this is more reliable and portable.

Suggestion

Avoid shell: true and pass args as an array to make the test more robust across platforms.

await expect(execa('email', ['check', templatePath], { cwd: __dirname }))
  .rejects.toMatchObject({ exitCode: 1 });

const { stdout } = await execa('email', ['check', templatePath, '--use-preview-props'], {
  cwd: __dirname,
});

Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.

Comment on lines +7 to +22
Copy link
Contributor Author

@charliecreates charliecreates bot Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This CLI test relies on shell: true and interpolates a file path into a shell command. While the path is test-controlled, using the shell can introduce quoting/escaping issues on Windows (spaces, special characters) and can also make the test environment-dependent.

It also depends on process.chdir(__dirname) at module load, which can cause cross-test interference if more CLI tests are added later.

Suggestion

Prefer running the CLI without a shell and avoid global chdir. For example:

const cwd = __dirname;
await expect(execa('email', ['check', templatePath], { cwd }))
  .rejects.toMatchObject({ exitCode: 1 });

const { stdout } = await execa('email', ['check', templatePath, '--use-preview-props'], { cwd });

and remove process.chdir(__dirname).

Reply with "@CharlieHelps yes please" if you’d like me to add a commit with this change.


const plain = strip(stdout);

expect(plain).toContain('Checking email template for Client Compatibility');
expect(plain).toContain('Check Complete');
expect(plain).not.toContain('noteLines');
expect(plain).not.toContain('asshole');
});
});
9 changes: 9 additions & 0 deletions test/cli/fixtures/check-preview-props.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
interface PreviewProps {
label: string;
}

export const previewProps: PreviewProps = {
label: 'Preview props loaded'
};

export const Template = ({ label }: PreviewProps) => <div>{label.toUpperCase()}</div>;
6 changes: 6 additions & 0 deletions test/cli/moon.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,12 @@ tasks:
# Naming this differently so it's not picked up bu the main test workflow
test.run:
command: vitest --config ../../shared/vitest.config.ts . --pool=forks
deps:
- plugin-inline:build
- plugin-minify:build
- plugin-pretty:build
- create-mail:build
- jsx-email:build
inputs:
- ./**/*.test.ts
- package.json
Expand Down
Loading