-
Notifications
You must be signed in to change notification settings - Fork 53
fix(jsx-email): normalize globby paths for preview (v3) #367
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
base: next/v3
Are you sure you want to change the base?
fix(jsx-email): normalize globby paths for preview (v3) #367
Conversation
… preview, config (#355) Co-authored-by: CharlieHelps <[email protected]>
…in (#358) Co-authored-by: CharlieHelps <[email protected]>
Co-authored-by: CharlieHelps <[email protected]>
…360) Co-authored-by: CharlieHelps <[email protected]>
…tional, tests (#361) Co-authored-by: CharlieHelps <[email protected]>
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.
No issues were found in the modified code. The change is minimal, aligned with the stated goal, and follows the existing patterns and repository instructions (notably [R1] about keeping diffs small and scoped). The use of normalizePath directly in the globby call is appropriate for cross-platform glob handling and does not introduce obvious performance or maintainability concerns.
Additional notes (1)
- Maintainability |
packages/jsx-email/src/cli/vite-static.ts:50-53
UsingnormalizePathon each configured glob before passing them toglobbyis the right way to make these patterns portable across Windows and POSIX, and it mirrors the existing pattern onmain. The transformation is local, side-effect free, and doesn’t alter runtime behavior for already-POSIX-style globs.
One potential follow-up (not required here per [R1]) would be to encapsulate the normalization into a small helper to make it obvious to future readers that paths are always treated as Vite-normalized POSIX patterns when used with globby, but given this PR’s narrow scope, keeping the inline map is acceptable and consistent with the existing minimal style.
Summary of changes
Summary of Changes
- Updated the Vite import in
packages/jsx-email/src/cli/vite-static.tsto includenormalizePathas a value import while keepingPluginOptionandViteDevServeras type imports. - Normalized configured
pathsbefore passing them toglobbyby mapping each path throughvite'snormalizePath()helper. - This aligns the
next/v3CLI static asset handling with the existing behavior onmain, improving Windows compatibility for backslash-separated asset globs while leaving non-Windows behavior unchanged.
Component / Package Name:
jsx-email / next/v3
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 #366
Description
Ports the Windows static-asset path fix from #365 onto the
next/v3branch soemail previewserves files correctly on Windows when--assetsglobs include backslash-separated paths.On
main, the fix normalized the globs passed toglobbyusing Vite'snormalizePath()helper. This PR applies the same normalization in thenext/v3CLI implementation:packages/jsx-email/src/cli/vite-static.tsto importnormalizePathfromvite.pathsbefore passing them toglobby, matching the behavior fromvite-static.mtsonmain.Runtime behavior for non-Windows platforms is unchanged; on Windows, static asset paths are now consistently treated as POSIX-style globs that
globbycan match.Verification
plugin-inline:build,plugin-minify:build, andplugin-pretty:buildall completed successfully.jsx-email:buildcompleted successfully.repo:lintcompleted with 31 existing warnings (in apps, docs, and tests) but no errors; none are related tovite-static.ts.jsx-email:testpassed: 46 test files, 190 tests.jsx-email:tsccurrently fails with a preexisting TS2717 error inpackages/jsx-email/src/components/conditional.tsxabout duplicate'jsx-email-cond'declarations. This failure reproduces onorigin/next/v3before applying thevite-static.tschange and is unchanged by this PR.Self-review notes
The automated self-review surfaced a number of maintainability items (pnpm workspace flags, shared tsconfig changes, CLI test reshaping, Playwright/diffable-html versions, smoke snapshots, etc.) that do not appear in this branch's diff when compared to
origin/next/v3.git diff --stat origin/next/v3...HEADshows only one touched file:packages/jsx-email/src/cli/vite-static.tspnpm-workspace.yaml,shared/tsconfig.base.json,shared/tsconfig.eslint.json, tests, or lockfiles as part of this PR.Those self-review items correspond to earlier
next/v3alignment work (for example #355, #358, #359, #360, #361) and are intentionally left as-is here to keep this change narrowly scoped to porting the Windows asset path fix from #365.