-
Notifications
You must be signed in to change notification settings - Fork 53
fix(jsx-email): clean check output + add --use-preview-props #378
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
fix(jsx-email): clean check output + add --use-preview-props #378
Conversation
- Remove placeholder/debug output from check Notes formatting - Add --use-preview-props support for email check - Add unit coverage for formatting + flag pass-through
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 CLI changes look correct and the new formatting is deterministic, but the new tests have reliability/cleanup issues: temp directories may be left behind when a test fails, and one test writes under process.cwd() rather than the OS temp directory. Additionally, heavy use of any in mocks/assertions weakens the regression protection for the new usePreviewProps plumbing.
Additional notes (2)
- Maintainability |
packages/jsx-email/test/cli/check.test.ts:16-30
This mock usesanyforbuildOptions/params and the test suite also relies onas anyforformatIssue()andcommand()inputs. The compiler passing is assumed, but theanyusage here reduces the value of the tests: they won’t catch accidental option shape regressions (the exact thing this PR is adding).
Given you're already importing command and buildTemplates, it's feasible to type the mock args without much overhead.
- Compatibility |
packages/jsx-email/src/cli/commands/check.ts:16-16
The command’s options schema acceptsusePreviewProps, but there’s no obvious wiring here that actually parses--use-preview-propsintoargv.usePreviewProps(it may be handled elsewhere, but within this file the only guarantee isassert(CheckOptionsStruct, argv)). If the argument parsing layer doesn’t auto-map kebab-case to camelCase, the flag will be documented but ineffective.
At minimum, this file should either:
- Explicitly document the expected argv key (
usePreviewProps) and/or - Add a small compatibility shim (
argv.usePreviewProps ??= argv['use-preview-props']) if your parser provides raw keys.
Summary of changes
What changed
email check CLI enhancements
- Added
--use-preview-propstoemail checkhelp output and option parsing viavalibot(optional(boolean())). - Plumbed
argv.usePreviewPropsthrough tobuildTemplates()viabuildOptions.usePreviewProps.
Deterministic notes formatting
- Replaced placeholder/debug output in
formatNotes()with stable, indented, dimmed note rendering. - Exported
formatNotesandformatIssuefor direct unit testing.
Tests & snapshots
- Added
packages/jsx-email/test/cli/check.test.tsto cover:- help text includes
--use-preview-props - no debug output / no placeholder strings in formatting
- forwarding of
usePreviewPropstobuildTemplates - integration-ish check that
build()respects preview props when enabled
- help text includes
- Updated several config-related snapshots to reflect normalized (non-ANSI) log template output.
| test('passes --use-preview-props through to buildTemplates', async () => { | ||
| const dir = await mkdtemp(join(os.tmpdir(), 'jsx-email-check-')); | ||
| const templatePath = join(dir, 'template.tsx'); | ||
| await writeFile(templatePath, 'export const Template = () => null;', 'utf8'); | ||
|
|
||
| const { buildTemplates } = await import('../../src/cli/commands/build.js'); | ||
|
|
||
| await command({ usePreviewProps: true } as any, [templatePath]); | ||
|
|
||
| expect(buildTemplates).toHaveBeenCalledWith({ | ||
| buildOptions: { showStats: false, usePreviewProps: true, writeToFile: false }, | ||
| targetPath: templatePath | ||
| }); | ||
| }); | ||
|
|
||
| test('build uses previewProps when usePreviewProps is enabled', async () => { | ||
| const dir = await mkdtemp(join(process.cwd(), '.tmp-jsx-email-preview-props-')); | ||
| const out = join(dir, 'out'); | ||
| const modulePath = join(dir, 'template.mjs'); | ||
|
|
||
| await writeFile( | ||
| modulePath, | ||
| [ | ||
| "import React from 'react';", | ||
| "export const previewProps = { name: 'Bruce' };", | ||
| "export const Template = (props) => React.createElement('p', null, props.name ?? 'missing');", | ||
| '' | ||
| ].join('\n'), | ||
| 'utf8' | ||
| ); | ||
|
|
||
| const withPreviewProps = await build({ | ||
| argv: { out, usePreviewProps: true, writeToFile: false } as any, | ||
| path: modulePath, | ||
| sourceFile: modulePath | ||
| }); | ||
|
|
||
| const withoutPreviewProps = await build({ | ||
| argv: { out, props: '{"name":"Clark"}', writeToFile: false } as any, | ||
| path: modulePath, | ||
| sourceFile: modulePath | ||
| }); | ||
|
|
||
| expect(withPreviewProps.html).toContain('Bruce'); | ||
| expect(withoutPreviewProps.html).toContain('Clark'); | ||
|
|
||
| await rm(dir, { force: true, recursive: true }); | ||
| }); |
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 new test suite creates temp directories, but cleanup is not guaranteed if an assertion fails (or if build() throws). This can leave .tmp-jsx-email-preview-props-* directories behind in local runs/CI workspaces.
Also, this test mixes os.tmpdir() (good) and process.cwd() (riskier) for temp dirs; prefer one consistent approach (ideally os.tmpdir()).
Suggestion
Wrap temp-dir lifecycle in try/finally (or use afterEach) and use os.tmpdir() consistently.
test('build uses previewProps when usePreviewProps is enabled', async () => {
const dir = await mkdtemp(join(os.tmpdir(), 'jsx-email-preview-props-'));
try {
const out = join(dir, 'out');
const modulePath = join(dir, 'template.mjs');
await writeFile(
modulePath,
[
"import React from 'react';",
"export const previewProps = { name: 'Bruce' };",
"export const Template = (props) => React.createElement('p', null, props.name ?? 'missing');",
''
].join('\n'),
'utf8'
);
const withPreviewProps = await build({
argv: { out, usePreviewProps: true, writeToFile: false } as any,
path: modulePath,
sourceFile: modulePath
});
const withoutPreviewProps = await build({
argv: { out, props: '{"name":"Clark"}', writeToFile: false } as any,
path: modulePath,
sourceFile: modulePath
});
expect(withPreviewProps.html).toContain('Bruce');
expect(withoutPreviewProps.html).toContain('Clark');
} finally {
await rm(dir, { force: true, recursive: true });
}
});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 #371
Description
On
next/v3,email checkhad placeholder/debug output informatNotes()and was missing--use-preview-propsparity.This PR:
--use-preview-propstoemail checkand passes it through tobuildTemplates().Verification
Self-review notes
reviewChangesflagged a set of warnings aboutpnpm-workspace.yaml,shared/tsconfig.*,scripts/ci-preview-setup.sh, andpnpm-lock.yaml. None of those files are changed in this PR; those warnings appear to be based onnext/v3branch divergence rather than this PR's diff.