build: modernize dev deps#711
Conversation
✅ Deploy Preview for remix-edge ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for remix-serverless ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR performs four independent developer tooling migrations alongside coordinated dependency version updates. The pre-commit hook switches from Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
6496aee to
a7eba5d
Compare
| "typescript": "^5.0.0", | ||
| "tinyglobby": "^0.2.15", | ||
| "typescript": "^6.0.3", | ||
| "unrun": "^0.3.1", |
There was a problem hiding this comment.
This allows us to use a .ts file for tsdown config on older versions of node.
We're dropping node 20 in the next PR, so this is super temporary.
There was a problem hiding this comment.
Let's ship #712 first and the we should be able to drop this from this PR to avoid temp additions to main?
Just leaving ref for others https://tsdown.dev/options/config-file#config-loaders - on "Node.js 22.18.0+" we won't need unrun and we are bumping min to Node 22.22
ecadb93 to
944d816
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/e2e/support/deploy-to-netlify.ts (1)
17-17: ⚡ Quick winAdd a timeout to
run()to prevent hanging CI jobs.
execFileAsynccurrently has no timeout; ifpnpm installorntl deployhangs, the E2E job can block indefinitely.Proposed fix
const run = (file: string, args: string[], cwd: string) => execFileAsync(file, args, { cwd, maxBuffer: MAX_BUFFER }) +const run = (file: string, args: string[], cwd: string) => + execFileAsync(file, args, { + cwd, + maxBuffer: MAX_BUFFER, + timeout: 10 * 60 * 1000, // 10 minutes + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/support/deploy-to-netlify.ts` at line 17, The run() function in deploy-to-netlify.ts is missing a timeout parameter in the execFileAsync call, which can cause E2E jobs to hang indefinitely if commands like pnpm install or ntl deploy stall. Add a timeout property to the options object passed to execFileAsync alongside the existing cwd and maxBuffer properties to ensure the command execution has a maximum time limit before it fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/e2e/support/deploy-to-netlify.ts`:
- Line 17: The run() function in deploy-to-netlify.ts is missing a timeout
parameter in the execFileAsync call, which can cause E2E jobs to hang
indefinitely if commands like pnpm install or ntl deploy stall. Add a timeout
property to the options object passed to execFileAsync alongside the existing
cwd and maxBuffer properties to ensure the command execution has a maximum time
limit before it fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0a010de5-d66e-48eb-aba2-2909cd5e7d81
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
.husky/pre-commit.lintstagedrc.js.nano-staged.jsonpackage.jsonpackages/remix-runtime/src/crypto.tspackages/remix-runtime/tsconfig.jsonpackages/vite-plugin-react-router/package.jsonpackages/vite-plugin-react-router/tsconfig.jsonpackages/vite-plugin-react-router/tsdown.config.mtspackages/vite-plugin-react-router/tsup.config.tstests/e2e/support/deploy-to-netlify.tstsconfig.deno.jsontsconfig.json
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
netlify/blueprints(manual)
💤 Files with no reviewable changes (2)
- .lintstagedrc.js
- packages/vite-plugin-react-router/tsup.config.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 944d816005
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
## Description React Router 8 has just been released. [React Router 8](https://reactrouter.com/upgrading/v7) makes the Vite Environment API mandatory and supports Vite 7-8. React Router 7 supports Vite 6-7 (and 8, starting with 7.14) but users can opt in to this mode as well via `future.v8_viteEnvironmentApi`. Either way, the Vite Env API is forward compatible from the "client" and "ssr" pseudo-environments present previously, and those are backwards compatible as well. The main change to contend with here is reading from `config.environments?.ssr?.build` instead of `config.ssr.build` directly. This is supported by all combinations of versions and flags listed above. **Edit:** I ended up doing this via the `configEnvironment` hook, which is equivalent but runs after React Router has actually configured its environments. We also replace `isSsrBuild` with a simple `applyToEnvironment` hook, which is compatible with all these combinations as well. Finally, this starts reading from `rolldownOptions` and falling back to `rollupOptions`. This was entirely optional, but since the latter is soft-deprecated, I introduced this here. Again, all combinations of versions and flags are compatible with this. Essentially: ``` Vite 5 Vite 6 Vite 7 Vite 8 RR7 EOL ✓ ✓ ✓ (Vite 8 requires RR 7.14.0+) RR8 EOL — ✓ ✓ ``` Now, unfortunately I ended up also needing to somewhat fundamentally rework the whole plugin's approach. The previous approach was to register our own input that wraps the (leakily) expected React Router input. This is now fundamentally incompatible with React Router 8 as soon as a site has any prerendered pages, because React Router 8 uses a separate Vite Environment `prerender` that it builds by first building the other envs, then starting a preview server, making real HTTP requests to each path to be prerendered, and saving the response to disk. This request was failing, seemingly due to some [assumption in React Router 8 that is not happy with its own entry coexisting with another](https://github.com/remix-run/react-router/blob/da102b5928d2cf0b94cd0abc2895d0939698e7e9/packages/react-router-dev/vite/plugin.ts#L3578-L3586). Funny enough, I more or less ended up just porting parts of our [framework-agnostic plugin](https://github.com/netlify/framework-adapters/blob/69d233d505afb85d848f702e70441bc3e71008bd/packages/vite-plugin/src/lib/build.ts) here. The bulk of the diff in this PR is adding two new fixtures for RR8 serverless + RR7 edge, and refactoring the existing test suite to allow running table tests with different versions of Vite. This was important because in testing this locally I contended with all sorts of different behaviours across combinations. This is a bit clunky and will slow down CI but I believe it is worthwhile given all this combinatorial complexity. Closes #698 ## Related Tickets & Documents See also #711 and #712
Upgrading TypeScript uncovered some errors in code that runs in Deno that aren't "real" errors, so I just fixed this setup instead.
- upgrade a bunch now that CI is fixed and we're dropping node 20 - use native node instead of heavy execa - use tinyglobby instead of heavier fast-glob - use tiny nano-staged instead of heavier lint-staged - use super fast tsdown, but only in maintained packages - use super fast tsgo preview
This allows us to use a `.ts` file for tsdown config on older versions of node. We're dropping node 20 in the next PR, so this is super temporary.
This reverts commit 8b9297f.
944d816 to
d7c6682
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@demos/vite-edge/package.json`:
- Around line 36-38: The typescript dependency version 6.0.3 does not match the
PR summary which mentions TypeScript 7 preview. Clarify and update the
typescript version in package.json to either stay at 6.0.3 or use the TypeScript
7 preview via the `@typescript/native-preview` package if you intend to use the
tsgo compiler. Additionally, remove the vite-tsconfig-paths dependency since
Vite 8.0.16 now has native support for TypeScript path mappings. Instead,
configure the resolve.tsconfigPaths option in your Vite configuration file to
enable this feature natively.
In `@packages/remix-adapter/package.json`:
- Around line 66-74: The tsup dependency at version ^8.5.1 in the
devDependencies has documented critical breaking issues including TypeScript
declaration file generation errors and watch mode rebuilding problems. Update
the tsup version constraint to a stable prior version that does not have these
issues, or alternatively consider migrating to tsdown if a replacement build
tool is preferred. Verify the chosen version is stable and does not exhibit the
reported TypeScript declaration or watch mode problems before committing the
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0efdc3e7-1e90-462c-b585-ed2c56fb79c4
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (18)
.husky/pre-commit.lintstagedrc.js.nano-staged.jsondemos/vite-edge/package.jsondemos/vite-functions/package.jsonpackage.jsonpackages/remix-adapter/package.jsonpackages/remix-edge-adapter/package.jsonpackages/remix-runtime/package.jsonpackages/remix-runtime/src/crypto.tspackages/remix-runtime/tsconfig.jsonpackages/vite-plugin-react-router/package.jsonpackages/vite-plugin-react-router/tsconfig.jsonpackages/vite-plugin-react-router/tsdown.config.mtspackages/vite-plugin-react-router/tsup.config.tstests/e2e/support/deploy-to-netlify.tstsconfig.deno.jsontsconfig.json
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
netlify/blueprints(manual)
💤 Files with no reviewable changes (2)
- packages/vite-plugin-react-router/tsup.config.ts
- .lintstagedrc.js
✅ Files skipped from review due to trivial changes (6)
- .husky/pre-commit
- tsconfig.deno.json
- tsconfig.json
- .nano-staged.json
- packages/remix-runtime/src/crypto.ts
- packages/remix-runtime/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/vite-plugin-react-router/tsconfig.json
- packages/vite-plugin-react-router/package.json
- tests/e2e/support/deploy-to-netlify.ts
- package.json
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8cd996bdb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This reverts commit e8cd996.
| format: ['esm'], | ||
| dts: true, | ||
| target: 'node22', | ||
| external, |
There was a problem hiding this comment.
seems like tsdown blessed way is to use deps.neverBundle (with external being supported, but deprecated)
packages/vite-plugin-react-router build: WARN `external` is deprecated. Use `deps.neverBundle` instead.
Description
Related Tickets & Documents
Follow-up to #709