fix(jsx-email): fix email check + --use-preview-props #389
Conversation
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
The CLI flag plumbing looks correct, but usePreviewProps is currently tri-state (true/false/undefined) and should be normalized to a deterministic boolean at the command boundary. The new CLI test relies on shell: true, which is less portable and more fragile than argument-based execa usage. The snapshot-stabilization logic is duplicated across multiple test files and should be centralized to prevent drift and ease maintenance.
Additional notes (1)
- Performance |
test/cli/moon.yml:10-17
depsfor the CLI test task ensures builds are available, but it can be heavier than necessary and may slow local iteration. Iftest/clionly needs a subset (likelyjsx-email:buildplus whatever provides theemailbinary), consider tightening dependencies to the minimum required to keep CI fast and reduce unnecessary rebuilds.
Summary of changes
What changed
jsx-email CLI: email check
- Added a new CLI option
--use-preview-propsand validated it viavalibot(optional(boolean())). - Updated help text to document
--use-preview-propsand show an example invocation. - Threaded
usePreviewPropsintobuildTemplates({ buildOptions: { … } }). - Replaced placeholder/debug output in
formatNotes()with a cleaned-up notes formatter that returns a properly indented, colored string.
Test stability + new CLI regression tests
- Refactored multiple config snapshot tests to dynamically import modules after resetting module state, and forced deterministic color/logging behavior.
- Added a new
test/cli/check.test.tsregression test coveringemail check --use-preview-props. - Added a fixture template exporting
previewProps(test/cli/fixtures/check-preview-props.tsx). - Updated
test/cli/moon.ymlto build required workspace dependencies before running CLI tests.
| const CheckOptionsStruct = object({ | ||
| usePreviewProps: optional(boolean()) | ||
| }); |
There was a problem hiding this comment.
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.
| 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'); | ||
|
|
There was a problem hiding this comment.
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.
| 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`; |
There was a problem hiding this comment.
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.
| 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`; |
There was a problem hiding this comment.
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.
| 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]; | ||
| } | ||
| }); |
There was a problem hiding this comment.
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.
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 #383
Description
email checkand formats caniemail notes cleanly.--use-preview-propssupport toemail checkand threads it intobuildTemplates()so templates can exportpreviewProps.test-cliregression suite foremail checkand updates thetest-cli:test.runMoon task to build required workspace deps.Verification
Self-review notes
pnpm-workspace.yaml,recipes/import-css/**, andtest/smoke/**, but those files are unchanged in this PR (likely stale context).