-
Notifications
You must be signed in to change notification settings - Fork 17
hotfix(rolldown): fix breaking change #395
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
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Pull Request Overview
This PR fixes a breaking change introduced by a recent rolldown upgrade by refactoring the server-side rendering code generation approach. Instead of generating unique variable names and capturing results, the code now directly returns values from the generated functions.
- Removed the dynamic SSR variable generation mechanism
- Updated
buildServerProgram
to use direct return statements instead of variable assignment - Simplified
executeServerCode
to work with the new return-based approach
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
src/generators/web/utils/processing.mjs | Removed SSR variable generation and updated function calls to match new API |
src/generators/web/utils/generate.mjs | Modified buildServerProgram to return values directly instead of assigning to variables |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #395 +/- ##
==========================================
+ Coverage 74.27% 74.30% +0.03%
==========================================
Files 118 118
Lines 11041 11031 -10
Branches 695 695
==========================================
- Hits 8201 8197 -4
+ Misses 2837 2831 -6
Partials 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Why did no CI catch this? Can we make sure we have whatever we need in CI so we can trust dependency upgrades? |
My assumption is that the script did fail (on Vercel), but was deployed despite the failure. We can prevent this from happening again by either:
|
👍 Can you make sure an issue is cut for that, I'd vote for doing both. |
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.
I do not claim to know what's happening in this logic, but the diff seems logical
For the record, in the rolldown update, tree-shaking was updated to shake unused variables, but no longer shake unused returns, so our logic (which previously accounted for shaked returns via variables) needed to be flipped |
@nodejs/web-infra Requesting fast-track.
The recent rolldown upgrade performed by Dependabot contained a breaking change. This PR updates our codebase to work with the new rolldown version.