-
Notifications
You must be signed in to change notification settings - Fork 28k
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
Improve Server Actions compiler #58391
Conversation
Tests Passed |
Stats from current PRDefault BuildGeneral
Client Bundles (main, webpack)
Legacy Client Bundles (polyfills)
Client Pages
Client Build Manifests
Rendered Page Sizes
Edge SSR bundle Size
Middleware size
Next Runtimes
|
} | ||
|
||
// In this example, if Button immediately executes the action, different ids should | ||
// be passed. |
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.
However if Button does not immediately execute the action then the same (double-incremented) id should be passed for both, which doesn't seem to be handled correctly with this compilation.
In addition, I believe we don't even let you run server actions at render time so I'm not sure it makes sense to optimize for that case?
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.
Another option might be to forbid capturing any mutable bindings and add a constraint that any captured variables must be assigned before the closure is referenced? Best practice is to have no interior mutability anywhere within props and arguably a captured let
that gets reassigned is a type of mutability.
But I would argue the current compilation here is incorrect.
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.
Yeah I later realized that we should just disallow mutating any captured variables, as it's impossible to pass any mutability across the boundary.
Currently to make inline-defined Server Actions work, the compiler hoists the actual
"use server"
function to the module level and convert the inlined function to a parentheses expression that creates a noop wrapper function and wraps it with the proxy. This works fine however expressions are still different from declarations (#57392). So there're some details that can't be aligned well.With this change, we're going to make the compilation for the two types of inline-defined Server Actions more robust and more lightweight:
1. Expressions
These expressions can directly be replaced with a new expression
createActionProxy("id", hoisted_action)
. A.bind(...)
member call can be followed if it needs to bind any variables from the closure level.2. Declarations
In this case, we replace all the same
named
idents to be the expressioncreateActionProxy("id", hoisted_action)
, and removed that function declaration.With these changes, these will be fewer structural changes to the AST and the code is more performant.
The PR also changes it to use React's
registerServerReference
method directly instead of our in-house implementation insidecreateActionProxy
.Another small change is to stabilize the comment header to use
BTreeMap
inside the SWC transform. Otherwise the test snapshots will randomly mismatch.Closes #57392.