diff --git a/.changeset/extract-nested-server-function-declarations.md b/.changeset/extract-nested-server-function-declarations.md new file mode 100644 index 000000000..d060f2ce6 --- /dev/null +++ b/.changeset/extract-nested-server-function-declarations.md @@ -0,0 +1,9 @@ +--- +"@solidjs/compiler": patch +--- + +A `"use server"` function declaration nested inside another function is now extracted like any other server function. + +- Previously the directive on a nested declaration was silently ignored: the body shipped to the client and ran there, while captures from the enclosing function were still rejected at compile time. +- The declaration is hoisted to a `const` at the top of its block, so calling it before its source position still works. +- Its id follows the binding path, such as `outer.inner`. diff --git a/packages/compiler/README.md b/packages/compiler/README.md index 3286bb021..f263f33ef 100644 --- a/packages/compiler/README.md +++ b/packages/compiler/README.md @@ -153,7 +153,7 @@ result.functions; // [{ id, name, exports }] for manifest building A function-level `"use server"` function is extracted out of its lexical position, so it may only reference its own parameters and locals, module top-level bindings, and globals. Capturing anything else is a compile error. That includes `this` and `arguments` in a marked arrow, which an arrow takes from the function it was written in. Use a `function` if the server function needs its own `this` or `arguments`. -A function-level directive only works where the pass can extract the function: a function declaration, a function expression, or an arrow with a block body. Methods, getters, and setters are never extracted, so a directive on one is a compile error rather than a directive that silently does nothing. Assign a function to a property instead. +A function-level directive only works where the pass can extract the function: a function declaration (at any nesting depth), a function expression, or an arrow with a block body. Methods, getters, and setters are never extracted, so a directive on one is a compile error rather than a directive that silently does nothing. Assign a function to a property instead. A module-level `"use server"` module can only export server functions. Its client build is rebuilt from those exports alone, so anything else would be missing from the browser bundle. Re-exports, `export *`, class and enum exports, destructured exports, and exports declared without an initializer are compile errors naming the export and its position. Type-only and `declare` exports are erased and are fine. diff --git a/packages/compiler/__tests__/directives-id-scheme.test.js b/packages/compiler/__tests__/directives-id-scheme.test.js index f6089ae8a..9f198696a 100644 --- a/packages/compiler/__tests__/directives-id-scheme.test.js +++ b/packages/compiler/__tests__/directives-id-scheme.test.js @@ -299,9 +299,10 @@ describe("the wire id scheme, differentially", () => { }); it("takes a segment from a nested function declaration", () => { - // Declarations nested inside another function are not bubbled into a - // `const`, so the function itself has to contribute the segment, or two - // same-named arrows in sibling declarations collide on `outer.s`. + // Two same-named arrows in sibling declarations must not collide on + // `outer.s`. Reported in post-bubble order (nested declarations hoist in + // reverse source order like top-level ones), which moves the reporting + // order without moving an id. const source = ` export function outer() { function a() { const s = async () => { "use server"; return 1; }; return s; } @@ -310,9 +311,23 @@ describe("the wire id scheme, differentially", () => { } `; const hash = hashHex("src/decl.js"); - expect(ids(source, { filename: "/project/src/decl.js", root: "/project" })).toEqual([ - `outer.a.s-${hash}`, - `outer.b.s-${hash}` + expect(ids(source, { filename: "/project/src/decl.js", root: "/project" }).sort()).toEqual( + [`outer.a.s-${hash}`, `outer.b.s-${hash}`].sort() + ); + }); + + it("names a marked nested function declaration by its path", () => { + // A declaration nested in another function is bubbled into a `const` + // like a top-level one, so it is extracted and named the same way. + const source = ` + export function outer() { + async function inner() { "use server"; return 1; } + return inner; + } + `; + const hash = hashHex("src/nested-decl.js"); + expect(ids(source, { filename: "/project/src/nested-decl.js", root: "/project" })).toEqual([ + `outer.inner-${hash}` ]); }); diff --git a/packages/compiler/__tests__/directives-nested-declarations.test.js b/packages/compiler/__tests__/directives-nested-declarations.test.js new file mode 100644 index 000000000..c7237ab40 --- /dev/null +++ b/packages/compiler/__tests__/directives-nested-declarations.test.js @@ -0,0 +1,155 @@ +// Function declarations nested inside another function carrying a +// function-level `"use server"` directive. +// +// The bubbling pre-pass turns every function declaration into +// `const name = function name() {}` at the top of its block so the transform +// only has to handle expression forms. The Babel original skipped the body of +// each bubbled declaration, so a declaration nested inside another function +// was never bubbled and a directive on it was silently ignored: the body +// shipped to the client and ran there, while the capture validator (which +// walks every function) still rejected its captures. These tests pin that a +// nested declaration is extracted like any other marked function, in both +// modes, and that hoisting still holds after the rewrite. + +const path = require("path"); + +const compilerDir = path.resolve(__dirname, ".."); +const { transformDirectives } = require(compilerDir); + +const RUNTIME = "@solidjs/web/server-functions"; +const ROOT = "/project"; +const FILENAME = `${ROOT}/src/module.js`; + +function compile(code, overrides = {}) { + return transformDirectives(code, { + filename: FILENAME, + root: ROOT, + mode: "server", + env: "production", + directive: "use server", + register: { kind: "named", name: "registerServerReference", source: RUNTIME }, + create: { kind: "named", name: "createServerReference", source: RUNTIME }, + ...overrides + }); +} + +const lines = code => code.split("\n").map(line => line.trim()); + +describe("nested function declarations", () => { + const source = [ + "export function outer() {", + " async function inner() {", + ' "use server";', + " return 1;", + " }", + " return inner;", + "}" + ].join("\n"); + + it("extracts a marked declaration nested in a function (server)", () => { + const result = compile(source); + expect(result.valid).toBe(true); + expect(result.functions.map(f => f.name)).toEqual(["outer.inner"]); + const out = lines(result.code); + // The body is registered at module top level and the binding inside + // `outer` becomes a reference to it, exactly like a top-level function. + expect(out).toContain( + 'const serverFunction_1 = registerServerReference_1("outer.inner-' + + hashOf(result) + + '", async function inner() {' + ); + expect(out).toContain("const inner = createServerReference_1(serverFunction_1);"); + expect(result.code).not.toMatch(/"use server"/); + }); + + it("replaces the declaration with a proxy on the client", () => { + const result = compile(source, { mode: "client" }); + expect(result.valid).toBe(true); + const out = lines(result.code); + expect(out).toContain( + `const inner = createServerReference_1("outer.inner-${hashOf(result)}");` + ); + // The server body is gone from the client build. + expect(result.code).not.toMatch(/return 1/); + expect(result.code).not.toMatch(/registerServerReference/); + }); + + it("keeps the declaration callable before its source position", () => { + // A function declaration is hoisted to the top of its scope. The bubbled + // `const` lands at the top of the block, so a use above the declaration + // still resolves after the rewrite. + const code = [ + "export function outer() {", + " const ref = inner;", + " async function inner() {", + ' "use server";', + " return 1;", + " }", + " return ref;", + "}" + ].join("\n"); + const out = lines(compile(code).code); + const declaration = out.indexOf("const inner = createServerReference_1(serverFunction_1);"); + const use = out.indexOf("const ref = inner;"); + expect(declaration).toBeGreaterThan(-1); + expect(use).toBeGreaterThan(declaration); + }); + + it("extracts through several levels of nesting", () => { + const code = [ + "export function outer() {", + " function mid() {", + ' async function inner() { "use server"; return 1; }', + " return inner;", + " }", + " return mid;", + "}" + ].join("\n"); + const result = compile(code); + expect(result.functions.map(f => f.name)).toEqual(["outer.mid.inner"]); + }); + + it("extracts a declaration inside a block inside a function", () => { + const code = [ + "export function outer(flag) {", + " if (flag) {", + ' async function inner() { "use server"; return 1; }', + " return inner;", + " }", + "}" + ].join("\n"); + const result = compile(code); + expect(result.functions.map(f => f.name)).toEqual(["outer.inner"]); + }); + + it("rejects a nested declaration that captures an enclosing local", () => { + // Extraction and validation agree: the same function that is now + // extracted is the one whose captures were already being checked. + const code = [ + "export function outer(db) {", + ' async function inner() { "use server"; return db.x; }', + " return inner;", + "}" + ].join("\n"); + expect(() => compile(code)).toThrow(/`db` is declared in an enclosing function/); + }); + + it("leaves an unmarked nested declaration's body in place", () => { + const code = [ + "export function outer() {", + " function helper() { return 2; }", + ' const go = async () => { "use server"; return 1; };', + " return [helper, go];", + "}" + ].join("\n"); + const result = compile(code, { mode: "client" }); + expect(result.functions.map(f => f.name)).toEqual(["outer.go"]); + expect(result.code).toMatch(/return 2/); + }); +}); + +// The file hash is whatever the pass produced; the scheme itself is pinned +// differentially in directives-id-scheme.test.js. +function hashOf(result) { + return result.functions[0].id.split("-")[1]; +} diff --git a/packages/compiler/src/directives/transform.rs b/packages/compiler/src/directives/transform.rs index d038ed8c5..187087920 100644 --- a/packages/compiler/src/directives/transform.rs +++ b/packages/compiler/src/directives/transform.rs @@ -702,13 +702,36 @@ impl<'a> DirectivesTransform<'a> { /// Babel's function-level pre-pass bubbles *every* function declaration to a /// `const` at the top of its enclosing block (so exports keep working and the -/// directive transform only has to handle expression forms). Declarations -/// nested inside another bubbled declaration are skipped, matching Babel's -/// `tmp.skip()`. +/// directive transform only has to handle expression forms). +/// +/// Babel skipped the body of each bubbled declaration (`tmp.skip()`), so a +/// declaration nested inside another function was never bubbled, never became +/// an expression, and a `"use server"` directive on it was silently ignored, +/// while the capture validator still treated it as a server function. This +/// pass descends into bubbled bodies too, so nested declarations are +/// extracted like every other marked function. Only files that extract at +/// least one function keep the pass's output, so the extra bubbling never +/// reaches a file without server functions. struct Bubbler<'ctx, 'a> { transform: &'ctx mut DirectivesTransform<'a>, } +impl<'ctx, 'a> Bubbler<'ctx, 'a> { + /// Bubbles the declarations inside `function`'s body, then turns the + /// declaration itself into `const name = function name() {}`. + fn bubble_declaration( + &mut self, + mut function: oxc_allocator::Box<'a, oxc_ast::ast::Function<'a>>, + ) -> Statement<'a> { + walk_mut::walk_function( + self, + &mut function, + oxc_syntax::scope::ScopeFlags::Function, + ); + self.transform.function_declaration_to_const(function) + } +} + impl<'a> VisitMut<'a> for Bubbler<'_, 'a> { fn visit_statements(&mut self, statements: &mut ArenaVec<'a, Statement<'a>>) { let ast = self.transform.ast(); @@ -718,7 +741,7 @@ impl<'a> VisitMut<'a> for Bubbler<'_, 'a> { for statement in old { match statement { Statement::FunctionDeclaration(function) if function.id.is_some() => { - hoisted.push(self.transform.function_declaration_to_const(function)); + hoisted.push(self.bubble_declaration(function)); } Statement::ExportDeclaration(export) if matches!( @@ -731,7 +754,7 @@ impl<'a> VisitMut<'a> for Bubbler<'_, 'a> { unreachable!("shape checked above"); }; let name = function.id.as_ref().unwrap().name.to_string(); - hoisted.push(self.transform.function_declaration_to_const(function)); + hoisted.push(self.bubble_declaration(function)); rest.push( self.transform .export_named_specifier_statement(&name, &name), @@ -753,7 +776,7 @@ impl<'a> VisitMut<'a> for Bubbler<'_, 'a> { unreachable!("shape checked above"); }; let name = function.id.as_ref().unwrap().name.to_string(); - hoisted.push(self.transform.function_declaration_to_const(function)); + hoisted.push(self.bubble_declaration(function)); export.declaration = ExportDefaultDeclarationKind::from(self.transform.identifier(&name)); rest.push(Statement::ExportDefaultDeclaration(export));