-
Notifications
You must be signed in to change notification settings - Fork 53
fix(create-mail,jsx-email,preview,test-cli): preview + scaffold #391
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
Conversation
Reorder imports and adjust spacing in documentation examples to match the updated create-mail scaffold conventions, keeping import groupings consistent across docs. Refs: #384
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 Windows drive-letter guard in preview.ts appears to check the wrong path (CLI --build-path instead of the temp build directory), which can make the workaround ineffective or produce false failures. The new CLI regression test also includes unconditional logging that will add noise to CI output. Addressing these will improve correctness and maintainability of the preview deploy path fixes.
Additional notes (2)
-
Maintainability |
packages/jsx-email/src/cli/commands/preview.ts:53-53
The Vite configresolve.aliasstill points@jsxemailbuildatbuildPath(the temp preview build dir), but--build-pathnow writes the deploy bundle tooutDir. This is likely intentional (alias is for runtime preview), but it creates an easy-to-miss divergence: users may expect@jsxemailbuildto correspond to the deploy output when running with--build-path. If any code paths (or future changes) reuse this config for deploy builds, it can accidentally reference the temp directory instead of the deploy directory. -
Compatibility |
packages/jsx-email/src/cli/vite-static.ts:50-56
Path normalization is applied when finding files (globby(paths.map(normalizePath))), but later request handling compares URL paths against filesystem paths. Iffilescontains normalized POSIX paths on Windows, ensure downstream comparisons (e.g.,endsWith,includes, or direct equality) also normalize consistently, otherwise requests may fail to match static files on Windows.
Summary of changes
Summary of changes
-
Preview app imports
- Normalized component import casing in the preview app (
Logo/Sidebar) by switching to./logoand../components/sidebar.
- Normalized component import casing in the preview app (
-
Docs updates
- Reordered imports and spacing in multiple docs to align examples with the
create-mailscaffold conventions.
- Reordered imports and spacing in multiple docs to align examples with the
-
create-mailscaffold- Added
react-domto scaffoldeddevDependenciesinpackages/create-mail/generators/package.json.mustache.
- Added
-
email preview --deploy/--build-pathfixes--build-pathnow defaults to./.deployand relative paths are resolved from the original working directory viaoriginalCwd.- Added Windows-specific guard to fail fast when Vite root and temp build directory are on different drive letters.
-
Static asset handling
- Normalized paths before
globby()invite-static.tsusing Vite’snormalizePath().
- Normalized paths before
-
Tests
- Extended
create-mailCLI test to assertreactandreact-domare indevDependenciesand not independencies. - Added a new CLI regression test for relative
--build-pathbehavior (and a Windows-specific error case).
- Extended
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new CLI test prints command output unconditionally via console.log(stdout). This adds noise to test runs/CI logs and can mask failures by making logs harder to read. Tests should only emit output when failing, or behind a debug flag.
Suggestion
Remove the console.log(stdout) line, or only print it when an assertion fails (e.g., wrap assertions in a try/catch and log on catch), or gate it behind an env var like DEBUG_CLI_TESTS.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
Component / Package Name:
create-mail / jsx-email / preview / test-cli
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 #384
Resolves #381
Resolves #366
Refs #379
Refs #390
Supersedes #385
Supersedes #387
Description
Combines the work from #385 and #387 into a single PR targeting
next/v3.create-mail: scaffolded projects now includereact-domindevDependencies.create-mailscaffolds.email preview --deploy: ported--build-path/ static asset fixes and added a CLI regression test for a relative--build-path.Verification
Self-review notes
The self-review tool surfaced maintainability suggestions in files not modified by this PR; leaving them unchanged to keep scope limited to combining #385 and #387.
scripts/ci-preview-setup.sh:14-32shared/tsconfig.base.json:1-16shared/tsconfig.eslint.json:1-16(file is already absent onnext/v3)shared/vitest.config.ts:1-10test/cli/create-mail.test.ts:1-34,test/cli/create-mail.test.ts:36-47,test/cli/create-mail.test.ts:49-48test/smoke/tests/smoke.test.ts:1-76pnpm-workspace.yaml:1-9recipes/import-css/package.json:1-20