fix(jsx-email): port preview deploy and asset-path fixes #387
Conversation
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
The main concerns are test robustness and long-term maintainability: describe(async () => ...) and console.log in test/cli/preview-build-path.test.ts can create flakiness/noisy CI output, and the test cleanup does not remove the created template directory. In preview.ts, the Windows drive-letter guard logic is good, but variable naming around buildPath is currently ambiguous and could lead to accidental misuse during future refactors. No functional regressions are obvious in the path normalization or import casing fixes.
Summary of changes
Summary
This PR ports preview deploy/static-asset fixes into next/v3 across the preview app and CLI:
-
Preview app (Linux/case-sensitive FS compatibility)
- Updated imports to match on-disk casing:
apps/preview/app/src/components/sidebar.tsx:./Logo→./logoapps/preview/app/src/layouts/Shell.tsx:../components/Sidebar→../components/sidebar
- Updated imports to match on-disk casing:
-
CLI preview deploy behavior (
packages/jsx-email/src/cli/commands/preview.ts)--build-pathhelp text updated and a relative path example added.--build-pathnow defaults to./.deployand relative paths are resolved against the original CWD viaoriginalCwd.- Removed
react/jsx-runtimefromrollupOptions.externalfor deployable builds. - Restored a Windows drive-letter guard to fail fast when Vite root and temp dir are on different drives.
-
Static asset glob normalization (
packages/jsx-email/src/cli/vite-static.ts)- Normalized
globby()patterns using Vite’snormalizePath()to avoid path separator issues.
- Normalized
-
Tests
- Added regression coverage for
email preview --build-pathintest/cli/preview-build-path.test.ts.
- Added regression coverage for
| describe('cli: preview --build-path', async () => { | ||
| const outRelational = './.test/build-path'; | ||
| const outAbsolute = resolve(__dirname, outRelational); | ||
| const templatePath = './.test/.deploy/emails'; | ||
| const isWindows = os.platform() === 'win32'; | ||
|
|
||
| beforeAll(async () => { | ||
| await rm(outAbsolute, { force: true, recursive: true }); | ||
| }); | ||
|
|
||
| afterAll(async () => { | ||
| await rm(outAbsolute, { force: true, recursive: true }); | ||
| }); | ||
|
|
||
| if (!isWindows) { | ||
| test('relative build path writes to an absolute path derived from the original cwd', async () => { | ||
| await execa({ cwd: __dirname, shell: true })`email create BatmanEmail --out ${templatePath} `; | ||
| const { stdout } = await execa({ | ||
| cwd: __dirname, | ||
| shell: true | ||
| })`email preview ${templatePath} --build-path ${outRelational}`; | ||
|
|
||
| console.log(stdout); | ||
|
|
||
| await access(join(outAbsolute, 'index.html')); | ||
|
|
||
| const html = await readFile(join(outAbsolute, 'index.html'), 'utf8'); | ||
| expect(html).toContain('<div id="root"></div>'); | ||
| }, 60e3); |
There was a problem hiding this comment.
This test suite uses describe(..., async () => { ... }). In Vitest/Jest-style runners, describe callbacks are expected to be synchronous; returning a Promise can lead to subtle scheduling issues and hooks not registering as expected.
Additionally, the test has a console.log(stdout) which will add noise to CI output and can hide real failures.
Finally, the email create ... step writes into templatePath, but the cleanup only removes outAbsolute. If the test runs multiple times (or on failure), .test/.deploy may accumulate files and create flakiness.
Suggestion
Make the describe callback synchronous, remove console.log, and clean up the template output directory as well.
describe('cli: preview --build-path', () => {
const outRelational = './.test/build-path';
const outAbsolute = resolve(__dirname, outRelational);
const templatePath = './.test/.deploy/emails';
const templateAbsolute = resolve(__dirname, templatePath);
beforeAll(async () => {
await rm(outAbsolute, { force: true, recursive: true });
await rm(templateAbsolute, { force: true, recursive: true });
});
afterAll(async () => {
await rm(outAbsolute, { force: true, recursive: true });
await rm(templateAbsolute, { force: true, recursive: true });
});
// ... remove console.log
});Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| if (!isWindows) { | ||
| test('relative build path writes to an absolute path derived from the original cwd', async () => { | ||
| await execa({ cwd: __dirname, shell: true })`email create BatmanEmail --out ${templatePath} `; | ||
| const { stdout } = await execa({ | ||
| cwd: __dirname, | ||
| shell: true | ||
| })`email preview ${templatePath} --build-path ${outRelational}`; | ||
|
|
||
| console.log(stdout); | ||
|
|
||
| await access(join(outAbsolute, 'index.html')); | ||
|
|
||
| const html = await readFile(join(outAbsolute, 'index.html'), 'utf8'); | ||
| expect(html).toContain('<div id="root"></div>'); | ||
| }, 60e3); |
There was a problem hiding this comment.
The non-Windows test invokes email preview ... --build-path ... and expects it to return (so it can access(index.html)). If email preview starts a server even when --build-path is provided (or if behavior changes), this test can hang/flap depending on CLI behavior.
It would be more robust to:
- Ensure the CLI path you're testing is a pure build-and-exit mode when
--build-pathis set, or - Add an explicit
--no-openand any build-only flag (if supported) to guarantee process termination.
This is especially important because you set a long timeout (60s) and use shell: true, which can mask process-lifecycle issues.
Suggestion
Harden the test to avoid hanging on long-running preview processes. If --build-path is intended to build and exit, assert that behavior explicitly by checking the exit code and/or by adding flags that guarantee exit.
Example:
const res = await execa({ cwd: __dirname, shell: true })
`email preview ${templatePath} --build-path ${outRelational} --no-open`;
expect(res.exitCode).toBe(0);If there is a dedicated build-only option (e.g. --build / --build-only), prefer that.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| if (isWindows) { | ||
| test('errors when Vite root and temp build path are on different drives (Windows)', async () => { | ||
| await execa({ cwd: __dirname, shell: true })`email create BatmanEmail --out ${templatePath} `; | ||
| await expect( | ||
| execa({ | ||
| cwd: __dirname, | ||
| shell: true | ||
| })`email preview ${templatePath} --build-path ${outRelational}` | ||
| ).rejects.toThrow( | ||
| /Temporary directory drive letter different than root directory drive letter/ | ||
| ); | ||
| }, 60e3); | ||
| } |
There was a problem hiding this comment.
The Windows branch expects a drive-letter mismatch error when running email preview ... --build-path <relative>. But --build-path is an output directory, and a relative path will resolve under the test cwd—it won’t naturally create a cross-drive condition.
If the intent is to validate the cross-drive failure mode, the test should either (a) simulate differing drives by controlling the temp directory location, or (b) unit-test the drive-letter guard logic with injected paths. As-is, this can be flaky or always-skip the real mismatch condition depending on environment.
Suggestion
Make the Windows test deterministic by decoupling it from the machine’s actual temp-drive layout.
Options:
- Refactor the guard into a small pure function and unit test it:
export const assertSameDriveOnWindows = (root: string, tempDir: string) => {
// ... throw AssertionError ...
};Then in CLI tests, validate the integration path uses the temp dir.
- If the code honors
TMPDIR/TEMP/TMPon Windows, set it for the spawned process to a path on a different drive (if available) and skip when not.
Reply with "@CharlieHelps yes please" if you’d like me to add a commit that refactors the guard for deterministic testing and updates this test accordingly.
|
Superseded by #391 (combined PR targeting next/v3). |
Component / Package Name:
jsx-email, preview
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 #381
resolves #366
#379
Description
Ports the post-merge-base
mainfixes intonext/v3for preview deploy builds + static assets:globby()patterns for preview static assets using Vite’snormalizePath().email preview --build-pathbehavior withmain:react/jsx-runtimefromrollupOptions.externalfor deployable preview builds.Sidebar/Logo) so deploy builds work on Linux.test/cli/preview-build-path.test.tsregression test.Verification
reviewChanges) returned comments for files not touched in this PR (e.g.pnpm-workspace.yaml,shared/tsconfig.base.json). I confirmed the diff for this PR is only:apps/preview/app/src/components/sidebar.tsxapps/preview/app/src/layouts/Shell.tsxpackages/jsx-email/src/cli/commands/preview.tspackages/jsx-email/src/cli/vite-static.tstest/cli/preview-build-path.test.ts