Skip to content

fix(jsx-email): port preview deploy and asset-path fixes #387

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 1 commit into
base: next/v3
Choose a base branch
from
Open

fix(jsx-email): port preview deploy and asset-path fixes #387

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion apps/preview/app/src/components/sidebar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { Link, useLocation } from 'react-router-dom';
import { useAppStore } from '../composables/useAppStore';
import type { TemplatePart } from '../lib/types';

import { Logo } from './Logo';
import { Logo } from './logo';
import { Separator } from './ui/Separator';

interface DirectoryTreeProps {
Expand Down
2 changes: 1 addition & 1 deletion apps/preview/app/src/layouts/Shell.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { Outlet } from 'react-router-dom';

import { Header, Sidebar } from '../components/Sidebar';
import { Header, Sidebar } from '../components/sidebar';

export const Shell = () => (
<>
Expand Down
36 changes: 30 additions & 6 deletions packages/jsx-email/src/cli/commands/preview.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
/* eslint-disable no-use-before-define */
import { AssertionError } from 'node:assert';
import { existsSync } from 'node:fs';
import { mkdir, rmdir } from 'node:fs/promises';
import { join, resolve } from 'node:path';
import os from 'node:os';
import { isAbsolute, join, resolve, win32 } from 'node:path';

import react from '@vitejs/plugin-react';
import chalk from 'chalk-template';
Expand All @@ -11,7 +13,7 @@ import { parse as assert } from 'valibot';
import { type InlineConfig, createServer, build as viteBuild } from 'vite';

import { log } from '../../log.js';
import { buildForPreview, writePreviewDataFiles } from '../helpers.js';
import { buildForPreview, originalCwd, writePreviewDataFiles } from '../helpers.js';
import { reloadPlugin } from '../vite-reload.js';
import { staticPlugin } from '../vite-static.js';
import { watch } from '../watcher.js';
Expand All @@ -37,14 +39,15 @@ Starts the preview server for a directory of email templates
$ email preview <template dir path> [...options]

{underline Options}
--build-path An absolute path. When set, builds the preview as a deployable app and saves to disk
--build-path When set, builds the preview as a deployable app and saves to disk
--exclude A micromatch glob pattern that specifies files to exclude from the preview
--host Allow thew preview server to listen on all addresses (0.0.0.0)
--no-open Do not open a browser tab when the preview server starts
--port The local port number the preview server should run on. Default: 55420

{underline Examples}
$ email preview ./src/templates --port 55420
$ email preview ./src/templates --build-path ./.deploy
$ email preview ./src/templates --build-path /tmp/email-preview
`;

Expand All @@ -55,19 +58,19 @@ const buildDeployable = async ({ argv, targetPath }: PreviewCommonParams) => {
);
}

const { basePath = './', buildPath } = argv;
const { basePath = './', buildPath = './.deploy' } = argv;
const common = { argv, targetPath };
await prepareBuild(common);
const config = await getConfig(common);
const outDir = isAbsolute(buildPath) ? buildPath : resolve(join(originalCwd, buildPath));

await viteBuild({
...config,
base: basePath,
build: {
minify: false,
outDir: buildPath,
outDir,
rollupOptions: {
external: ['react/jsx-runtime'],
output: {
manualChunks: {}
}
Expand All @@ -94,6 +97,22 @@ const getConfig = async ({ argv, targetPath }: PreviewCommonParams) => {

log.debug(`Vite Root: ${root}`);

// On Windows, Vite's import.meta.glob cannot cross drive letters. If the
// preview app root and the temporary build directory are on different
// drives (e.g., D: vs C:), fail fast with a helpful error.
if (os.platform() === 'win32') {
const rootDrive = getDriveLetter(root);
const buildDrive = getDriveLetter(buildPath);
if (rootDrive && buildDrive && rootDrive !== buildDrive) {
log.error(
`jsx-email preview cannot run on Windows when the application root directory and the system temporary directory are on different drive letters. Please consider using WSL`
);
throw new AssertionError({
message: `Temporary directory drive letter different than root directory drive letter`
});
}
}

newline();
log.info(chalk`{blue Starting build...}`);

Expand Down Expand Up @@ -127,6 +146,11 @@ const getConfig = async ({ argv, targetPath }: PreviewCommonParams) => {
return config;
};

const getDriveLetter = (path: string) => {
if (os.platform() !== 'win32') return null;
return win32.parse(path).root.slice(0, 2).toUpperCase();
};

const prepareBuild = async ({ targetPath, argv }: PreviewCommonParams) => {
const buildPath = await getTempPath('preview');
const { exclude } = argv;
Expand Down
4 changes: 2 additions & 2 deletions packages/jsx-email/src/cli/vite-static.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { extname } from 'node:path';

import { globby } from 'globby';
import mime from 'mime-types';
import type { PluginOption, ViteDevServer } from 'vite';
import { type PluginOption, type ViteDevServer, normalizePath } from 'vite';

interface ViteStaticOptions {
paths: string[];
Expand Down Expand Up @@ -50,7 +50,7 @@ interface MiddlwareParams {
const middleware = async (params: MiddlwareParams) => {
const { options, server } = params;
const { paths } = options;
const files = await globby(paths);
const files = await globby(paths.map((path) => normalizePath(path)));

return () => {
server.middlewares.use(async (req, res, next) => {
Expand Down
51 changes: 51 additions & 0 deletions test/cli/preview-build-path.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import { access, readFile, rm } from 'node:fs/promises';
import os from 'node:os';
import { join, resolve } from 'node:path';

import { execa } from 'execa';

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);
Comment on lines +7 to +35
Copy link
Contributor Author

@charliecreates charliecreates bot Dec 9, 2025

Choose a reason for hiding this comment

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

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.

Comment on lines +21 to +35
Copy link
Contributor Author

@charliecreates charliecreates bot Dec 9, 2025

Choose a reason for hiding this comment

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

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-path is set, or
  • Add an explicit --no-open and 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);
}
Comment on lines +38 to +50
Copy link
Contributor Author

@charliecreates charliecreates bot Dec 9, 2025

Choose a reason for hiding this comment

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

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:

  1. 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.

  1. If the code honors TMPDIR/TEMP/TMP on 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.

});