-
Notifications
You must be signed in to change notification settings - Fork 53
chore(repo): update typescript, vite, vitest #344
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
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.
buffer as anyincompile.tsunnecessarily discards type safety; usebuffer.toString('utf8')or cast asUint8Arrayforesbuild.transforminstead.- Replacing
--no-threadswith--pool=forksmay change test semantics; add--workers=1if single-worker execution is desired for deterministic CLI tests. - The rest of the changes (Vitest config migration, snapshot updates, and task references) look consistent with the dependency upgrades.
Additional notes (1)
- Maintainability |
packages/jsx-email/package.json:100-100
The lockfile currently includes multipleesbuildversions (e.g.,0.24.xand0.25.x). Thepackages/jsx-emailpackage still pinsesbuildto^0.24.0, which may be causing duplication and inconsistent typings across the repo. Aligning to a single modernesbuildversion can reduce install size and avoid type drift.
Summary of changes
- Bumped core tooling: TypeScript to
^5.9.3, Vitest to^4.0.5, Vite to^7.1.12, and@vitejs/plugin-reactto^5.1.0. - Migrated Vitest config from
shared/vitest.config.tsto ESMshared/vitest.config.mtsand updated all references (Moon tasks and docs/playbooks). - Updated test task flags for compatibility with Vitest 4 (using
--pool=forks). - Large snapshot refresh to match new serializers/formatting from the upgraded test stack.
- Minor code change in
packages/jsx-email/src/renderer/compile.tsusingbuffer as anywhen callingesbuild.transform. - Synchronized ESLint TS config include to the new Vitest config filename.
- pnpm lockfile updated to reflect the dependency bumps throughout the monorepo.
| builder.onLoad({ filter: /\.css$/ }, async (args) => { | ||
| const buffer = await readFile(args.path); | ||
| const css = await esbuild.transform(buffer, { loader: 'css', minify: false }); | ||
| const css = await esbuild.transform(buffer as any, { loader: 'css', minify: false }); |
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.
Avoid using as any here; it suppresses type safety and can hide real issues. esbuild.transform accepts a string or Uint8Array, and readFile returns a Buffer (which is a Uint8Array). Prefer passing a proper type instead of erasing it.
Two safer options:
- Convert to string explicitly (simplest for CSS):
buffer.toString('utf8') - Cast to
Uint8Array(no copy):buffer as Uint8Array
Converting to string is the clearest and avoids surprises with encoding-sensitive transforms.
Suggestion
Replace the any cast with a strongly-typed input to esbuild.transform.
Example:
const source = buffer.toString('utf8');
const css = await esbuild.transform(source, { loader: 'css', minify: false });or, if you prefer not to allocate a string:
const css = await esbuild.transform(buffer as Uint8Array, { loader: 'css', minify: false });Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| "typescript": "^5.9.3", | ||
| "vitest": "^4.0.5" |
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.
You’ve upgraded to Vite 7 and @vitejs/plugin-react@5.1.0, which both require modern Node (Vite 7 requires Node >= 20.19). To prevent accidental CI or local runs on older Node versions, declare an explicit engines constraint at the repo root (and/or enforce via Volta/nvm).
Suggestion
Consider adding an engines field to the root package.json to enforce the minimum Node version required by Vite 7 and its plugins:
{
"engines": {
"node": ">=20.19.0"
}
}I can add this in a tiny follow-up commit if you’d like. Reply with "@CharlieHelps yes please" and I’ll push it.
Component / Package Name:
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:
Description
Test dependency updates