From ccde7b4859a303c03269b9b67d08e4a288b0f61b Mon Sep 17 00:00:00 2001 From: "Alexis H. Munsayac" Date: Fri, 11 Sep 2026 14:26:13 +0800 Subject: [PATCH 1/2] fix(directives): report server functions that cannot work A server function is moved to the top level of its module, and several shapes stop working once it is. They compiled without complaint before. - Fail the build when a server function reads a variable from an enclosing function, uses `this` or `arguments` in an arrow, uses `super`, or reads a private class member. - Fail the build when the directive is in an object or class method. The transform ignored it, which shipped the method body and the modules it imports to the browser. - Warn when a directive string is not the first statement of a module or a function body, where it has no effect. - Warn for each export a "use server" module cannot serve, naming it. Those exports are still left out of the client build. - Support an anonymous default export from a "use server" module. - Compile server functions in `.mts` and `.cts` files. - Point build errors at the full path of the file. Co-Authored-By: Claude Opus 5 --- .changeset/server-function-diagnostics.md | 14 + packages/start/src/directives/compile.spec.ts | 322 ++++++++++++++++ packages/start/src/directives/compile.ts | 13 +- packages/start/src/directives/index.ts | 9 +- packages/start/src/directives/plugin.ts | 354 ++++++++++-------- packages/start/src/directives/validate.ts | 224 +++++++++++ 6 files changed, 780 insertions(+), 156 deletions(-) create mode 100644 .changeset/server-function-diagnostics.md create mode 100644 packages/start/src/directives/validate.ts diff --git a/.changeset/server-function-diagnostics.md b/.changeset/server-function-diagnostics.md new file mode 100644 index 000000000..fdc01e250 --- /dev/null +++ b/.changeset/server-function-diagnostics.md @@ -0,0 +1,14 @@ +--- +"@solidjs/start": patch +--- + +Report server functions that cannot work instead of compiling them into broken output. + +- A `"use server"` function that reads a variable from an enclosing function now fails the build. The function is moved to the top level of its module, so the variable is not in scope when it runs. +- The same check covers `this` and `arguments` in an arrow function, `super`, and private class members. +- A `"use server"` directive in an object or class method now fails the build. It was ignored before, which shipped the method body and the modules it imports to the browser. +- A `"use server"` string that is not the first statement of a module or a function body now logs a warning. It has no effect there. +- An export a `"use server"` module cannot serve now logs a warning that names it. These exports are still left out of the client build. +- A `"use server"` module can now export an anonymous default function. Both `export default async () => {}` and `export default async function () {}` work. +- Server functions are now compiled in `.mts` and `.cts` files. +- Build errors now point at the full path of the file, not just its name. diff --git a/packages/start/src/directives/compile.spec.ts b/packages/start/src/directives/compile.spec.ts index 876a0c7b6..d35ddc33e 100644 --- a/packages/start/src/directives/compile.spec.ts +++ b/packages/start/src/directives/compile.spec.ts @@ -60,3 +60,325 @@ describe("compile", () => { expect(result.code).not.toMatch(/\bverify\b/); }); }); + +const serverOptions: CompileOptions = { ...clientOptions, mode: "server" }; + +function compileBoth(code: string, id = "/src/server-action.ts") { + return Promise.all([compile(id, code, clientOptions), compile(id, code, serverOptions)]); +} + +describe("unsupported server functions", () => { + it("rejects a directive in an object method", async () => { + await expect( + compile( + "/src/api.ts", + `export const api = { async read() { "use server"; return 1; } };`, + clientOptions, + ), + ).rejects.toThrow(/not supported in a method/); + }); + + it("rejects a directive in a class method", async () => { + await expect( + compile( + "/src/api.ts", + `export class Api { async read() { "use server"; return 1; } }`, + clientOptions, + ), + ).rejects.toThrow(/not supported in a method/); + }); + + it("rejects a directive in a getter", async () => { + await expect( + compile( + "/src/api.ts", + `const api = { get value() { "use server"; return 1; } };`, + clientOptions, + ), + ).rejects.toThrow(/not supported in a method/); + }); + + it("rejects a value captured from an enclosing function", async () => { + await expect( + compile( + "/src/counter.ts", + `export function makeCounter(start) { + return async () => { "use server"; return start; }; + }`, + clientOptions, + ), + ).rejects.toThrow(/"start" is declared outside/); + }); + + it("rejects a value captured from a surrounding block", async () => { + await expect( + compile( + "/src/handlers.ts", + `export function handlers(items) { + return items.map(item => async () => { "use server"; return item; }); + }`, + clientOptions, + ), + ).rejects.toThrow(/"item" is declared outside/); + }); + + it("rejects `this` in an arrow inside a class", async () => { + await expect( + compile( + "/src/api.ts", + `export class Api { + x = 1; + handler = async () => { "use server"; return this.x; }; + }`, + clientOptions, + ), + ).rejects.toThrow(/`this` cannot be used/); + }); + + it("rejects `arguments` in an arrow inside a function", async () => { + await expect( + compile( + "/src/api.ts", + `export function outer() { + return async () => { "use server"; return arguments.length; }; + }`, + clientOptions, + ), + ).rejects.toThrow(/`arguments` cannot be used/); + }); + + it("rejects `super`", async () => { + await expect( + compile( + "/src/api.ts", + `export class Api extends Object { + read() { return async () => { "use server"; return super.toString(); }; } + }`, + clientOptions, + ), + ).rejects.toThrow(/`super` cannot be used/); + }); + + it("rejects a private class member", async () => { + await expect( + compile( + "/src/api.ts", + `export class Api { + #secret = 1; + read() { return async () => { "use server"; return this.#secret; }; } + }`, + clientOptions, + ), + ).rejects.toThrow(/private class member/); + }); +}); + +describe("supported server functions", () => { + it("allows module scope, globals, parameters and locals", async () => { + const code = ` + import { db } from "./db.ts"; + const table = "users"; + export const load = async (id: string) => { + "use server"; + const query = \`select * from \${table}\`; + return db.run(query, id, Date.now()); + }; + `; + const [client, server] = await compileBoth(code); + expect(client.valid).toBe(true); + expect(server.valid).toBe(true); + }); + + it("allows `this` and `arguments` in a function expression", async () => { + const code = ` + export function outer() { + return function () { + "use server"; + return [this, arguments.length]; + }; + } + `; + const [client] = await compileBoth(code); + expect(client.valid).toBe(true); + }); + + it("does not read type annotations as captured values", async () => { + const code = ` + export function outer() { + type Local = { id: T }; + return async (value: Local): Promise => { + "use server"; + return value; + }; + } + `; + const [client] = await compileBoth(code); + expect(client.valid).toBe(true); + }); +}); + +describe('"use server" modules', () => { + it("supports an anonymous default export", async () => { + const [client, server] = await compileBoth( + `"use server";\nexport default async () => 1;`, + "/src/action.ts", + ); + expect(client.code).toContain('export { fn_1 as "default" }'); + expect(server.code).toContain("createServerReference"); + // both sides have to agree on the id + const id = /cloneServerReference_1\("([^"]+)"\)/.exec(client.code)?.[1]; + expect(id).toBeTruthy(); + expect(server.code).toContain(`"${id}"`); + }); + + it("supports an anonymous default function declaration", async () => { + const [client, server] = await compileBoth( + `"use server";\nexport default async function () { return 1; }`, + "/src/action.ts", + ); + expect(client.code).toContain('export { fn_1 as "default" }'); + expect(server.code).toContain("createServerReference"); + }); + + it("keeps ids aligned when a module exports several functions", async () => { + const code = `"use server"; + export const first = async () => 1; + export default async () => 2; + export const second = async () => 3; + `; + const [client, server] = await compileBoth(code, "/src/actions.ts"); + const ids = [...client.code.matchAll(/cloneServerReference_1\("([^"]+)"\)/g)].map( + match => match[1]!, + ); + expect(ids).toHaveLength(3); + for (const id of ids) { + expect(server.code).toContain(`createServerReference_1("${id}"`); + } + }); + + it("allows type-only exports", async () => { + const code = `"use server"; + export type Session = { id: string }; + export const load = async () => 1; + `; + const [client] = await compileBoth(code, "/src/actions.ts"); + expect(client.valid).toBe(true); + }); + + it("reports a non-function export instead of dropping it silently", async () => { + const result = await compile( + "/src/actions.ts", + `"use server";\nexport const NAME = "constant";\nexport const fn = async () => 1;`, + clientOptions, + ); + expect(result.code).not.toMatch(/\bNAME\b/); + expect(result.warnings[0]).toMatch(/left out of the client build/); + }); + + it("still drops a wrapped function, which cannot be recognised statically", async () => { + const code = `"use server"; + import { query } from "@solidjs/router"; + export const testQuery = query(() => 1, "testQuery"); + `; + const [client] = await compileBoth(code, "/src/actions.ts"); + expect(client.code).not.toMatch(/\btestQuery\b/); + expect(client.warnings).toHaveLength(1); + }); + + it("reports a class export", async () => { + const result = await compile( + "/src/actions.ts", + `"use server";\nexport class Service {}\nexport const fn = async () => 1;`, + clientOptions, + ); + expect(result.warnings[0]).toMatch(/Only functions can be exported/); + }); + + it("reports a re-export", async () => { + const result = await compile( + "/src/actions.ts", + `"use server";\nexport { helper } from "./helper.ts";\nexport const fn = async () => 1;`, + clientOptions, + ); + expect(result.warnings[0]).toMatch(/Re-exporting from another module/); + }); + + it("reports `export * from`", async () => { + const result = await compile( + "/src/actions.ts", + `"use server";\nexport * from "./other.ts";\nexport const fn = async () => 1;`, + clientOptions, + ); + expect(result.warnings[0]).toMatch(/export \* from/); + }); + + it("reports a destructured export", async () => { + const result = await compile( + "/src/actions.ts", + `"use server";\nconst obj = { a: 1, b: 2 };\nexport const { a, b } = obj;`, + clientOptions, + ); + expect(result.warnings[0]).toMatch(/Destructured exports/); + }); + + it("reports a non-function default export", async () => { + const result = await compile( + "/src/actions.ts", + `"use server";\nexport default 42;`, + clientOptions, + ); + expect(result.warnings[0]).toMatch(/default export is not a function/); + }); + + it("keeps ids aligned when an unsupported export sits between two functions", async () => { + const code = `"use server"; + export const first = async () => 1; + export const NAME = "constant"; + export const second = async () => 2; + `; + const [client, server] = await compileBoth(code, "/src/actions.ts"); + const ids = [...client.code.matchAll(/cloneServerReference_1\("([^"]+)"\)/g)].map( + match => match[1]!, + ); + expect(ids).toHaveLength(2); + for (const id of ids) { + expect(server.code).toContain(`createServerReference_1("${id}"`); + } + }); +}); + +describe("misplaced directives", () => { + it("warns when the directive is not the first statement of a module", async () => { + const result = await compile( + "/src/actions.ts", + `import { thing } from "./thing.ts";\n"use server";\nexport const fn = async () => thing();`, + clientOptions, + ); + expect(result.valid).toBe(false); + expect(result.warnings).toHaveLength(1); + expect(result.warnings[0]).toMatch(/is ignored because it is not the first statement/); + }); + + it("warns when the directive is not the first statement of a function", async () => { + const result = await compile( + "/src/actions.ts", + `export const fn = async () => { + const now = Date.now(); + "use server"; + return now; + };`, + clientOptions, + ); + expect(result.valid).toBe(false); + expect(result.warnings[0]).toMatch(/is ignored because it is not the first statement/); + }); + + it("does not warn for a correctly placed directive", async () => { + const result = await compile( + "/src/actions.ts", + `export const fn = async () => { "use server"; return 1; };`, + clientOptions, + ); + expect(result.warnings).toHaveLength(0); + }); +}); diff --git a/packages/start/src/directives/compile.ts b/packages/start/src/directives/compile.ts index 774280e44..b1f54c5b2 100644 --- a/packages/start/src/directives/compile.ts +++ b/packages/start/src/directives/compile.ts @@ -1,5 +1,4 @@ import * as babel from "@babel/core"; -import path from "node:path"; import { directivesPlugin, type StateContext } from "./plugin.ts"; import xxHash32 from "./xxhash32.ts"; @@ -7,9 +6,14 @@ export interface CompileResult { valid: boolean; code: string; map: babel.BabelFileResult["map"]; + /** Problems that do not stop the compile, reported to the caller. */ + warnings: string[]; } -export type CompileOptions = Omit; +export type CompileOptions = Omit< + StateContext, + "count" | "hash" | "imports" | "valid" | "warnings" +>; export async function compile( id: string, @@ -19,6 +23,7 @@ export async function compile( const context: StateContext = { ...options, valid: false, + warnings: [], hash: xxHash32(id).toString(16), count: 0, imports: new Map(), @@ -35,7 +40,8 @@ export async function compile( parserOpts: { plugins, }, - filename: path.basename(id), + // The full path, so diagnostics point at the file the user edits. + filename: id, ast: false, sourceMaps: true, configFile: false, @@ -48,6 +54,7 @@ export async function compile( valid: context.valid, code: result.code || "", map: result.map, + warnings: context.warnings, }; } throw new Error("invariant"); diff --git a/packages/start/src/directives/index.ts b/packages/start/src/directives/index.ts index 40273b4f8..9e438b61f 100644 --- a/packages/start/src/directives/index.ts +++ b/packages/start/src/directives/index.ts @@ -23,8 +23,9 @@ export interface ServerFunctionsOptions { filter?: ServerFunctionsFilter; } -const DEFAULT_INCLUDE = "src/**/*.{jsx,tsx,ts,js,mjs,cjs}"; -const DEFAULT_EXCLUDE = "node_modules/**/*.{jsx,tsx,ts,js,mjs,cjs}"; +const EXTENSIONS = "{jsx,tsx,ts,js,mjs,cjs,mts,cts}"; +const DEFAULT_INCLUDE = `src/**/*.${EXTENSIONS}`; +const DEFAULT_EXCLUDE = `node_modules/**/*.${EXTENSIONS}`; const DIRECTIVE = "use server"; // Dev-only virtual module used by fns/handler.ts to lazily resolve a server @@ -242,6 +243,10 @@ export function serverFunctionsPlugin(options: ServerFunctionsOptions): Plugin[] env, }); + for (const warning of result.warnings) { + this.warn(warning); + } + if (result.valid) { const preloader = preload[mode]; if (preloader) { diff --git a/packages/start/src/directives/plugin.ts b/packages/start/src/directives/plugin.ts index 20299c294..3baed7c05 100644 --- a/packages/start/src/directives/plugin.ts +++ b/packages/start/src/directives/plugin.ts @@ -10,6 +10,11 @@ import { isStatementTopLevel } from "./is-statement-top-level.ts"; import { isPathValid, unwrapPath } from "./paths.ts"; import { removeUnusedVariables } from "./remove-unused-variables.ts"; import type { ImportDefinition } from "./types.ts"; +import { + assertHoistable, + assertNoMethodDirectives, + collectMisplacedDirectives, +} from "./validate.ts"; export interface StateContext { env: "production" | "development"; @@ -19,6 +24,7 @@ export interface StateContext { count: number; imports: Map; valid: boolean; + warnings: string[]; definitions: { register: ImportDefinition; @@ -95,6 +101,9 @@ function transformFunction( } cleanFunctionDirectives(ctx, path); } + // The function is about to be moved to the top level of the module, so + // everything it reads has to still resolve from there. + assertHoistable(path, ctx.directive); // First, get root statement const rootStatement = getRootStatementPath(path); @@ -166,186 +175,226 @@ function traceBinding(path: babel.NodePath, name: string): Binding | undefined { } } -function transformBindingForServer(ctx: StateContext, binding: Binding) { - if (isPathValid(binding.path, t.isVariableDeclarator)) { - const right = unwrapPath(binding.path.get("init"), isValidFunction); - if (right) { - transformFunction(ctx, right, true); - } +interface ModuleExport { + names: string[]; + path: babel.NodePath; +} + +/** + * An export a `"${directive}"` module cannot serve is left out of the client + * build, so client code importing it gets a missing binding. The transform + * cannot tell a wrapped function such as `query(fn, key)` from a plain value, + * so this reports the export instead of failing the build. + */ +function droppedExport(ctx: StateContext, path: babel.NodePath, detail: string): void { + const line = path.node.loc?.start.line; + ctx.warnings.push( + `A "${ctx.directive}" module can only serve the functions it declares, so this export is left out of the client build` + + `${line == null ? "" : ` (line ${line})`}. ${detail}`, + ); +} + +function isTypeOnlyDeclaration(node: t.Node | null | undefined): boolean { + switch (node?.type) { + case "TSInterfaceDeclaration": + case "TSTypeAliasDeclaration": + case "TSDeclareFunction": + case "TSModuleDeclaration": + return true; + default: + return false; } } -interface State extends babel.PluginPass { - opts: StateContext; +function resolveExportedFunction( + path: babel.NodePath, + name: string, +): babel.NodePath | undefined { + const binding = traceBinding(path, name); + if (binding && isPathValid(binding.path, t.isVariableDeclarator)) { + return unwrapPath(binding.path.get("init"), isValidFunction); + } + return undefined; } -function transformModuleLevelDirective(ctx: StateContext, program: babel.NodePath) { - cleanDirectives(program, ctx.directive); +/** + * Every function a `"use server"` module exports, in source order, with the + * names it is exported under. Both modes walk this list in the same order, + * which is what keeps the ids generated on the two sides pointing at each + * other. + */ +function collectModuleExports( + ctx: StateContext, + program: babel.NodePath, +): ModuleExport[] { + const entries: ModuleExport[] = []; + const byNode = new Map(); + + function add(name: string, path: babel.NodePath): void { + const existing = byNode.get(path.node); + if (existing) { + existing.names.push(name); + return; + } + const entry: ModuleExport = { names: [name], path }; + byNode.set(path.node, entry); + entries.push(entry); + } + program.traverse({ - FunctionDeclaration(child) { - // We only need to move top-level functions - if (isStatementTopLevel(child)) { - bubbleFunctionDeclaration(child); + ExportAllDeclaration(path) { + if (path.node.exportKind === "type") { + return; } + droppedExport( + ctx, + path, + '"export * from" re-exports nothing the client can call. Re-export from a module without the directive instead.', + ); }, - }); - program.scope.crawl(); - if (ctx.mode === "server") { - // Trace bindings - const bindings = new Set(); - - program.traverse({ - ExportDefaultDeclaration(path) { - const id = unwrapPath(path.get("declaration"), t.isIdentifier); - if (id) { - const binding = traceBinding(path, id.node.name); - if (binding) { - bindings.add(binding); - } + ExportDefaultDeclaration(path) { + const declaration = path.get("declaration"); + const id = unwrapPath(declaration, t.isIdentifier); + if (id) { + const fn = resolveExportedFunction(path, id.node.name); + if (fn) { + add("default", fn); + } else { + droppedExport(ctx, path, `"${id.node.name}" is not a function declared in this module.`); } - }, - ExportNamedDeclaration(path) { - if (path.node.source || path.node.exportKind === "type") { - return; + return; + } + // `export default async function () {}` is only a declaration by + // position and has no binding to trace, so read it as an expression. + if (isPathValid(declaration, t.isFunctionDeclaration) && !declaration.node.id) { + const node = declaration.node; + declaration.replaceWith( + t.functionExpression(null, node.params, node.body, node.generator, node.async), + ); + } + const fn = unwrapPath(path.get("declaration"), isValidFunction); + if (fn) { + add("default", fn); + } else { + droppedExport(ctx, path, "The default export is not a function declared in this module."); + } + }, + ExportNamedDeclaration(path) { + if (path.node.exportKind === "type") { + return; + } + if (path.node.source) { + droppedExport( + ctx, + path, + "Re-exporting from another module is not supported. Export a function declared here that calls it instead.", + ); + return; + } + for (const specifier of path.get("specifiers")) { + if (!isPathValid(specifier, t.isExportSpecifier)) { + droppedExport(ctx, specifier, "Only named exports of functions are supported."); + continue; } - for (const specifier of path.get("specifiers")) { - if (isPathValid(specifier, t.isExportSpecifier)) { - const binding = traceBinding(specifier, specifier.node.local.name); - - if (binding) { - bindings.add(binding); - } - } + if (specifier.node.exportKind === "type") { + continue; } - const declarations = path.get("declaration"); - - if (isPathValid(declarations, t.isVariableDeclaration)) { - for (const declaration of declarations.get("declarations")) { - // Check if left is identifier - const left = unwrapPath(declaration.get("id"), t.isIdentifier); - if (left) { - const binding = traceBinding(left, left.node.name); - if (binding) { - bindings.add(binding); - } - } - } + const local = specifier.node.local.name; + const fn = resolveExportedFunction(specifier, local); + if (!fn) { + droppedExport(ctx, specifier, `"${local}" is not a function declared in this module.`); + continue; } - }, - }); + const exported = specifier.node.exported; + add(t.isIdentifier(exported) ? exported.name : exported.value, fn); + } - for (const binding of bindings) { - transformBindingForServer(ctx, binding); - } - } else { - // Trace bindings - const uniqueBindings = new Set(); - const exportedBindings = new Map(); - - program.traverse({ - ExportDefaultDeclaration(path) { - const id = unwrapPath(path.get("declaration"), t.isIdentifier); - if (id) { - const binding = traceBinding(path, id.node.name); - if (binding) { - uniqueBindings.add(binding); - exportedBindings.set("default", binding); - } - } - }, - ExportNamedDeclaration(path) { - if (path.node.source || path.node.exportKind === "type") { - return; - } - for (const specifier of path.get("specifiers")) { - if (isPathValid(specifier, t.isExportSpecifier)) { - const binding = traceBinding(specifier, specifier.node.local.name); - - if (binding) { - const key = t.isIdentifier(specifier.node.exported) - ? specifier.node.exported.name - : specifier.node.exported.value; - uniqueBindings.add(binding); - exportedBindings.set(key, binding); - } + const declaration = path.get("declaration"); + if (isPathValid(declaration, t.isVariableDeclaration)) { + for (const declarator of declaration.get("declarations")) { + const left = unwrapPath(declarator.get("id"), t.isIdentifier); + if (!left) { + droppedExport(ctx, declarator, "Destructured exports are not supported."); + continue; } - } - - const declarations = path.get("declaration"); - - if (isPathValid(declarations, t.isVariableDeclaration)) { - for (const declaration of declarations.get("declarations")) { - // Check if left is identifier - const left = unwrapPath(declaration.get("id"), t.isIdentifier); - if (left) { - const binding = traceBinding(left, left.node.name); - if (binding) { - uniqueBindings.add(binding); - exportedBindings.set(left.node.name, binding); - } - } + const fn = resolveExportedFunction(left, left.node.name); + if (!fn) { + droppedExport(ctx, declarator, `"${left.node.name}" is not a function.`); + continue; } + add(left.node.name, fn); } - }, - }); - - // generate ids for each unique binding - const sourceIDs = new Map(); - for (const binding of uniqueBindings) { - if (isPathValid(binding.path, t.isVariableDeclarator)) { - const init = unwrapPath(binding.path.get("init"), isValidFunction); - if (init) { - sourceIDs.set(binding, createID(ctx, getDescriptiveName(init, "anonymous"))); - } + } else if ( + declaration.node && + !isPathValid(declaration, t.isFunctionDeclaration) && + !isTypeOnlyDeclaration(declaration.node) + ) { + droppedExport(ctx, declaration as babel.NodePath, "Only functions can be exported."); } - } + }, + }); - // clear body - program.node.body = []; + return entries; +} - const declarations: t.VariableDeclarator[] = []; - const specifiers: t.ExportSpecifier[] = []; +function transformModuleLevelDirective(ctx: StateContext, program: babel.NodePath) { + cleanDirectives(program, ctx.directive); + program.traverse({ + FunctionDeclaration(child) { + // We only need to move top-level functions + if (isStatementTopLevel(child)) { + bubbleFunctionDeclaration(child); + } + }, + }); + program.scope.crawl(); - const declarationMap = new Map(); + const entries = collectModuleExports(ctx, program); - // Declare all client functions - for (const [exported, binding] of exportedBindings) { - let currentIdentifier = declarationMap.get(binding); - if (!currentIdentifier) { - currentIdentifier = generateUniqueName(program, "fn"); + if (ctx.mode === "server") { + for (const entry of entries) { + transformFunction(ctx, entry.path, true); + } + return; + } - const fnID = sourceIDs.get(binding); + const ids = entries.map(entry => createID(ctx, getDescriptiveName(entry.path, "anonymous"))); - if (fnID) { - declarations.push( - t.variableDeclarator( - currentIdentifier, - t.callExpression(getImportIdentifier(ctx.imports, program, ctx.definitions.clone), [ - t.stringLiteral(fnID), - ]), - ), - ); + // clear body + program.node.body = []; - declarationMap.set(binding, currentIdentifier); - } - } + const declarations: t.VariableDeclarator[] = []; + const specifiers: t.ExportSpecifier[] = []; - if (currentIdentifier) { - specifiers.push(t.exportSpecifier(currentIdentifier, t.stringLiteral(exported))); - } + for (let i = 0, len = entries.length; i < len; i++) { + const local = generateUniqueName(program, "fn"); + declarations.push( + t.variableDeclarator( + local, + t.callExpression(getImportIdentifier(ctx.imports, program, ctx.definitions.clone), [ + t.stringLiteral(ids[i]!), + ]), + ), + ); + for (const name of entries[i]!.names) { + specifiers.push(t.exportSpecifier(local, t.stringLiteral(name))); } + } - const body: t.Statement[] = []; - - if (declarations.length > 0) { - body.push(t.variableDeclaration("const", declarations)); - } - if (specifiers.length > 0) { - body.push(t.exportNamedDeclaration(null, specifiers, null)); - } + const body: t.Statement[] = []; - program.pushContainer("body", body); + if (declarations.length > 0) { + body.push(t.variableDeclaration("const", declarations)); } + if (specifiers.length > 0) { + body.push(t.exportNamedDeclaration(null, specifiers, null)); + } + + program.pushContainer("body", body); +} + +interface State extends babel.PluginPass { + opts: StateContext; } export function directivesPlugin(): babel.PluginObj { @@ -353,6 +402,9 @@ export function directivesPlugin(): babel.PluginObj { name: "solid-start:directives", visitor: { Program(program, ctx) { + assertNoMethodDirectives(program, ctx.opts.directive); + ctx.opts.warnings.push(...collectMisplacedDirectives(program, ctx.opts.directive)); + const isModuleLevel = isDirectiveValid(ctx.opts, program.node.directives); if (isModuleLevel) { transformModuleLevelDirective(ctx.opts, program); diff --git a/packages/start/src/directives/validate.ts b/packages/start/src/directives/validate.ts new file mode 100644 index 000000000..aff3a5219 --- /dev/null +++ b/packages/start/src/directives/validate.ts @@ -0,0 +1,224 @@ +import type * as babel from "@babel/core"; +import * as t from "@babel/types"; + +type HoistableFunction = t.ArrowFunctionExpression | t.FunctionExpression; + +/** + * A server function is moved to the top level of its module. Anything it reads + * from an enclosing scope stops resolving once it is moved. These checks report + * that at build time. Without them the code compiles and then fails at runtime, + * or stops parsing. + */ + +function isTypeOnlyPosition(path: babel.NodePath, boundary: babel.NodePath): boolean { + let current: babel.NodePath | null = path; + while (current && current !== boundary) { + if (current.node.type.startsWith("TS") || current.node.type.startsWith("Type")) { + return true; + } + current = current.parentPath; + } + return false; +} + +/** + * The nearest ancestor that decides what `this` and `arguments` mean. + * Arrow functions are transparent. Every other function form is not. + */ +function getThisBoundary(path: babel.NodePath): babel.NodePath | null { + let previous: babel.NodePath = path; + let current: babel.NodePath | null = path.parentPath; + while (current) { + switch (current.node.type) { + case "FunctionExpression": + case "FunctionDeclaration": + case "ObjectMethod": + case "ClassMethod": + case "ClassPrivateMethod": + case "StaticBlock": + case "Program": + return current; + case "ClassProperty": + case "ClassPrivateProperty": + // Only the initializer is bound to the instance. + if (previous.node === current.node.value) { + return current; + } + break; + default: + break; + } + previous = current; + current = current.parentPath; + } + return null; +} + +function findAncestor( + path: babel.NodePath, + boundary: babel.NodePath, + types: string[], +): babel.NodePath | null { + let current: babel.NodePath | null = path.parentPath; + while (current && current !== boundary) { + if (types.includes(current.node.type)) { + return current; + } + current = current.parentPath; + } + return null; +} + +export function assertHoistable(path: babel.NodePath, directive: string): void { + const program = path.scope.getProgramParent().path; + // A function expression keeps its own `this` and `arguments` wherever it is + // moved. An arrow takes both from where it is written. + const isArrow = path.node.type === "ArrowFunctionExpression"; + const boundary = getThisBoundary(path); + // An arrow written at the top level already reads `this` from the module. + const lexicalSelfIsModule = boundary === null || boundary.node.type === "Program"; + + /** + * Whether `this` or `arguments` at this position is bound by something that + * stays behind when the server function moves. A function expression keeps + * both wherever it lands, and a nested function binds its own. + */ + function isLexicallyOutside(child: babel.NodePath): boolean { + const binder = getThisBoundary(child); + if (binder && binder !== path && binder.isDescendant(path)) { + return false; + } + if (binder === path && !isArrow) { + return false; + } + return !lexicalSelfIsModule; + } + + function unsupported(target: babel.NodePath, what: string, hint: string): never { + throw target.buildCodeFrameError( + `${what} cannot be used inside a "${directive}" function, because the function is moved to the top level of the module. ${hint}`, + ); + } + + // `super` and private members are reported first. When both apply to the same + // expression, such as `this.#value`, they are the more specific cause. + path.traverse({ + Super(child) { + const home = findAncestor(child, path, ["ObjectMethod", "ClassMethod", "ClassPrivateMethod"]); + if (!home) { + unsupported( + child, + "`super`", + "Move the server function out of the method and pass what it needs as an argument.", + ); + } + }, + PrivateName(child) { + const owner = findAncestor(child, path, ["ClassBody"]); + if (!owner) { + unsupported( + child, + "A private class member", + "Read it outside the server function and pass it as an argument.", + ); + } + }, + }); + + path.traverse({ + ThisExpression(child) { + if (!isLexicallyOutside(child)) { + return; + } + unsupported(child, "`this`", "Pass the value it refers to as an argument instead."); + }, + Identifier(child) { + if (!child.isReferencedIdentifier() || isTypeOnlyPosition(child, path)) { + return; + } + const { name } = child.node; + + if (name === "arguments") { + if (!isLexicallyOutside(child)) { + return; + } + unsupported( + child, + "`arguments`", + "Declare the parameters the server function needs instead.", + ); + } + + const binding = child.scope.getBinding(name); + // No binding means a global, which is still a global after the move. + if (!binding) { + return; + } + // Declared by the function itself, so it moves along with it. + if (binding.path === path || binding.path.isDescendant(path)) { + return; + } + // Declared at the top level of the module, where the function lands. + if (binding.scope.path === program) { + return; + } + throw child.buildCodeFrameError( + `"${name}" is declared outside the "${directive}" function that uses it, and the function is moved to the top level of the module, so "${name}" is not in scope when it runs. Pass it as an argument instead.`, + ); + }, + }); +} + +/** + * A directive only applies to a function body. The transform ignores one in a + * method, which ships the method body and every module it imports to the + * browser. + */ +export function assertNoMethodDirectives( + program: babel.NodePath, + directive: string, +): void { + function check( + child: babel.NodePath, + ): void { + for (const current of child.node.body.directives) { + if (current.value.value === directive) { + throw child.buildCodeFrameError( + `"${directive}" is not supported in a method. Move the body into a function and call it from the method:\n` + + ` const handler = async () => { "${directive}"; /* ... */ };`, + ); + } + } + } + program.traverse({ + ObjectMethod: check, + ClassMethod: check, + ClassPrivateMethod: check, + }); +} + +/** + * A directive string that is not in a directive prologue does nothing. It is + * almost always meant to be one, so report it. Otherwise the module compiles as + * if it had no server functions. + */ +export function collectMisplacedDirectives( + program: babel.NodePath, + directive: string, +): string[] { + const warnings: string[] = []; + program.traverse({ + ExpressionStatement(child) { + const expression = child.node.expression; + if (!t.isStringLiteral(expression) || expression.value !== directive) { + return; + } + const line = child.node.loc?.start.line; + warnings.push( + `"${directive}" on line ${line ?? "?"} is ignored because it is not the first statement of a function body or of the module. ` + + `Move it to the top of the body, before any other statement.`, + ); + }, + }); + return warnings; +} From b0ddee2233e2a47de5a4695a8063b7373b0e3bd1 Mon Sep 17 00:00:00 2001 From: "Alexis H. Munsayac" Date: Fri, 11 Sep 2026 14:34:45 +0800 Subject: [PATCH 2/2] fix(directives): build server function ids from nested names Ids were the position of the function in the file, so adding a server function renumbered every later one. A client bundle from an earlier build then called a different function instead of failing. - Build the id from the names the function is nested under, such as `Page.load`. Functions that share a name path are numbered. - Keep production ids opaque by hashing that name. - Drop `get-descriptive-name.ts`, which nothing uses now. Co-Authored-By: Claude Opus 5 --- .changeset/server-function-diagnostics.md | 1 + packages/start/src/directives/compile.spec.ts | 106 ++++++++++++++++ packages/start/src/directives/compile.ts | 3 +- .../src/directives/get-descriptive-name.ts | 39 ------ .../src/directives/get-hierarchical-name.ts | 113 ++++++++++++++++++ packages/start/src/directives/plugin.ts | 24 ++-- 6 files changed, 239 insertions(+), 47 deletions(-) delete mode 100644 packages/start/src/directives/get-descriptive-name.ts create mode 100644 packages/start/src/directives/get-hierarchical-name.ts diff --git a/.changeset/server-function-diagnostics.md b/.changeset/server-function-diagnostics.md index fdc01e250..702a65587 100644 --- a/.changeset/server-function-diagnostics.md +++ b/.changeset/server-function-diagnostics.md @@ -10,5 +10,6 @@ Report server functions that cannot work instead of compiling them into broken o - A `"use server"` string that is not the first statement of a module or a function body now logs a warning. It has no effect there. - An export a `"use server"` module cannot serve now logs a warning that names it. These exports are still left out of the client build. - A `"use server"` module can now export an anonymous default function. Both `export default async () => {}` and `export default async function () {}` work. +- Server function ids are now built from the names a function is nested under, such as `Page.load`, instead of the order the functions appear in. An id no longer changes when another server function is added to the same file, and two functions that share a name are told apart by the names around them. Production ids stay opaque. - Server functions are now compiled in `.mts` and `.cts` files. - Build errors now point at the full path of the file, not just its name. diff --git a/packages/start/src/directives/compile.spec.ts b/packages/start/src/directives/compile.spec.ts index d35ddc33e..3a8d51217 100644 --- a/packages/start/src/directives/compile.spec.ts +++ b/packages/start/src/directives/compile.spec.ts @@ -382,3 +382,109 @@ describe("misplaced directives", () => { expect(result.warnings).toHaveLength(0); }); }); + +describe("server function ids", () => { + async function idsOf(code: string, options: CompileOptions = clientOptions) { + const result = await compile("/src/routes/page.tsx", code, options); + return [...result.code.matchAll(/cloneServerReference_1\("([^"]+)"\)/g)].map( + match => match[1]!, + ); + } + + it("names a function by where it sits in the file", async () => { + const [id] = await idsOf(` + export function Page() { + const load = async () => { "use server"; return 1; }; + return load; + } + `); + expect(id).toMatch(/-Page\.load$/); + }); + + it("tells apart two functions that share a name", async () => { + const ids = await idsOf(` + export function Page() { + return async () => { "use server"; return 1; }; + } + export function Admin() { + return async () => { "use server"; return 2; }; + } + export const load = async () => { "use server"; return 3; }; + `); + // Bubbling reorders the output, so compare the set of ids. + expect(ids.sort()).toEqual([ + expect.stringMatching(/-Admin\.anonymous$/), + expect.stringMatching(/-Page\.anonymous$/), + expect.stringMatching(/-load$/), + ]); + }); + + it("numbers functions that share the same name path", async () => { + const ids = await idsOf(` + export const pair = register( + async () => { "use server"; return 1; }, + async () => { "use server"; return 2; }, + ); + `); + expect(ids).toEqual([expect.stringMatching(/-pair$/), expect.stringMatching(/-pair\$1$/)]); + }); + + it("keeps ids of existing functions when a function is added above them", async () => { + const before = await idsOf(` + export const load = async () => { "use server"; return 1; }; + export const save = async () => { "use server"; return 2; }; + `); + const after = await idsOf(` + export const added = async () => { "use server"; return 0; }; + export const load = async () => { "use server"; return 1; }; + export const save = async () => { "use server"; return 2; }; + `); + expect(after).toContain(before[0]); + expect(after).toContain(before[1]); + }); + + it("keeps client and server ids aligned around nested server functions", async () => { + const code = ` + export const outer = register(async () => { + "use server"; + return register(async () => { "use server"; return 1; }); + }); + export const beside = register(async () => { "use server"; return 2; }); + `; + const [client, server] = await compileBoth(code, "/src/routes/page.tsx"); + const ids = [...client.code.matchAll(/cloneServerReference_1\("([^"]+)"\)/g)].map( + match => match[1]!, + ); + expect(ids).toHaveLength(2); + for (const id of ids) { + expect(server.code).toContain(`createServerReference_1("${id}"`); + } + }); + + it("does not ship source names in production ids", async () => { + const production: CompileOptions = { ...clientOptions, env: "production" }; + const ids = await idsOf( + `export function Page() { + const load = async () => { "use server"; return 1; }; + return load; + }`, + production, + ); + expect(ids[0]).not.toMatch(/Page|load/); + expect(ids[0]).toMatch(/^[0-9a-f]+-[0-9a-f]+$/); + }); + + it("keeps production ids stable when a function is added above them", async () => { + const production: CompileOptions = { ...clientOptions, env: "production" }; + const before = await idsOf( + `export const load = async () => { "use server"; return 1; };`, + production, + ); + const after = await idsOf( + `export const added = async () => { "use server"; return 0; }; + export const load = async () => { "use server"; return 1; };`, + production, + ); + expect(after).toContain(before[0]); + }); +}); diff --git a/packages/start/src/directives/compile.ts b/packages/start/src/directives/compile.ts index b1f54c5b2..9b6b368d7 100644 --- a/packages/start/src/directives/compile.ts +++ b/packages/start/src/directives/compile.ts @@ -12,7 +12,7 @@ export interface CompileResult { export type CompileOptions = Omit< StateContext, - "count" | "hash" | "imports" | "valid" | "warnings" + "count" | "names" | "hash" | "imports" | "valid" | "warnings" >; export async function compile( @@ -26,6 +26,7 @@ export async function compile( warnings: [], hash: xxHash32(id).toString(16), count: 0, + names: new Map(), imports: new Map(), }; const pluginOption = [directivesPlugin, context]; diff --git a/packages/start/src/directives/get-descriptive-name.ts b/packages/start/src/directives/get-descriptive-name.ts deleted file mode 100644 index 9a856a998..000000000 --- a/packages/start/src/directives/get-descriptive-name.ts +++ /dev/null @@ -1,39 +0,0 @@ -import type { NodePath } from "@babel/core"; - -export function getDescriptiveName(path: NodePath, defaultName: string): string { - let current: NodePath | null = path; - while (current) { - switch (current.node.type) { - case "FunctionDeclaration": - case "FunctionExpression": { - if (current.node.id) { - return current.node.id.name; - } - break; - } - case "VariableDeclarator": { - if (current.node.id.type === "Identifier") { - return current.node.id.name; - } - break; - } - case "ClassPrivateMethod": - case "ClassMethod": - case "ObjectMethod": { - switch (current.node.key.type) { - case "Identifier": - return current.node.key.name; - case "PrivateName": - return current.node.key.id.name; - default: - break; - } - break; - } - default: - break; - } - current = current.parentPath; - } - return defaultName; -} diff --git a/packages/start/src/directives/get-hierarchical-name.ts b/packages/start/src/directives/get-hierarchical-name.ts new file mode 100644 index 000000000..9e8cfc98b --- /dev/null +++ b/packages/start/src/directives/get-hierarchical-name.ts @@ -0,0 +1,113 @@ +import type { NodePath } from "@babel/core"; +import * as t from "@babel/types"; + +/** + * The names a function is nested under, from the top of the module down, joined + * with a dot. `Page.load` for a `load` declared inside `Page`. + * + * Server function ids are built from this instead of the order the functions + * appear in. An id then stays the same when another function is added to the + * file, and two functions that share a name are still told apart by the names + * around them. + */ + +const ANONYMOUS = "anonymous"; + +function getKeyName(node: t.Node, computed: boolean): string | undefined { + if (computed) { + return undefined; + } + switch (node.type) { + case "Identifier": + return node.name; + case "PrivateName": + return node.id.name; + case "StringLiteral": + return node.value; + case "NumericLiteral": + return String(node.value); + default: + return undefined; + } +} + +function getSegment(path: NodePath, child: NodePath): string | undefined { + const node = path.node; + switch (node.type) { + case "VariableDeclarator": + return t.isIdentifier(node.id) ? node.id.name : undefined; + case "FunctionDeclaration": + case "ClassDeclaration": + case "ClassExpression": + return node.id?.name; + case "FunctionExpression": + if (node.id) { + return node.id.name; + } + return path.parentPath && getSegment(path.parentPath, path) ? undefined : ANONYMOUS; + case "ObjectProperty": + return getKeyName(node.key, node.computed); + case "ObjectMethod": + case "ClassMethod": + return getKeyName(node.key, node.computed ?? false); + case "ClassPrivateMethod": + return getKeyName(node.key, false); + case "ClassProperty": + // Only the initializer belongs to the property. + return child.node === node.value ? getKeyName(node.key, node.computed ?? false) : undefined; + case "ClassPrivateProperty": + return child.node === node.value ? getKeyName(node.key, false) : undefined; + case "ExportDefaultDeclaration": + return "default"; + case "AssignmentExpression": + return t.isIdentifier(node.left) ? node.left.name : undefined; + case "ArrowFunctionExpression": + // An enclosing function that nothing names still has to separate what is + // inside it from what is beside it. + return path.parentPath && getSegment(path.parentPath, path) ? undefined : ANONYMOUS; + default: + return undefined; + } +} + +function isFunctionBoundary(node: t.Node): boolean { + switch (node.type) { + case "ArrowFunctionExpression": + case "FunctionExpression": + case "FunctionDeclaration": + case "ObjectMethod": + case "ClassMethod": + case "ClassPrivateMethod": + case "StaticBlock": + return true; + default: + return false; + } +} + +export function getHierarchicalName(path: NodePath): string { + const segments: string[] = []; + let child: NodePath = path; + let current: NodePath | null = path.parentPath; + + while (current && !t.isProgram(current.node)) { + // Nothing named this function before another function encloses it, so it + // is an anonymous function inside that one. + if (segments.length === 0 && isFunctionBoundary(current.node)) { + segments.push(ANONYMOUS); + } + const segment = getSegment(current, child); + // A named function expression assigned to a variable of the same name + // would otherwise repeat itself. + if (segment && segment !== segments[segments.length - 1]) { + segments.push(segment); + } + child = current; + current = current.parentPath; + } + + if (segments.length === 0) { + return ANONYMOUS; + } + return segments.reverse().join("."); +} diff --git a/packages/start/src/directives/plugin.ts b/packages/start/src/directives/plugin.ts index 3baed7c05..1e512bd33 100644 --- a/packages/start/src/directives/plugin.ts +++ b/packages/start/src/directives/plugin.ts @@ -3,13 +3,14 @@ import type { Binding } from "@babel/traverse"; import * as t from "@babel/types"; import { bubbleFunctionDeclaration } from "./bubble-function-declaration.ts"; import { generateUniqueName } from "./generate-unique-name.ts"; -import { getDescriptiveName } from "./get-descriptive-name.ts"; +import { getHierarchicalName } from "./get-hierarchical-name.ts"; import { getImportIdentifier } from "./get-import-identifier.ts"; import { getRootStatementPath } from "./get-root-statement-path.ts"; import { isStatementTopLevel } from "./is-statement-top-level.ts"; import { isPathValid, unwrapPath } from "./paths.ts"; import { removeUnusedVariables } from "./remove-unused-variables.ts"; import type { ImportDefinition } from "./types.ts"; +import xxHash32 from "./xxhash32.ts"; import { assertHoistable, assertNoMethodDirectives, @@ -22,6 +23,8 @@ export interface StateContext { directive: string; hash: string; count: number; + /** How many times each name path has been used, to keep ids unique. */ + names: Map; imports: Map; valid: boolean; warnings: string[]; @@ -82,12 +85,19 @@ function isFunctionDirectiveValid( return false; } -function createID(ctx: StateContext, name: string) { - const base = `${ctx.hash}-${ctx.count++}`; +function createID(ctx: StateContext, path: babel.NodePath) { + const name = getHierarchicalName(path); + // Two functions can still share a name path, such as two arrows passed to + // the same call, so repeats are numbered. + const seen = ctx.names.get(name) ?? 0; + ctx.names.set(name, seen + 1); + const unique = seen === 0 ? name : `${name}$${seen}`; + ctx.count++; if (ctx.env === "development") { - return `${base}-${name}`; + return `${ctx.hash}-${unique}`; } - return base; + // Production ids stay opaque, so source names are not shipped to the browser. + return `${ctx.hash}-${xxHash32(unique).toString(16)}`; } function transformFunction( @@ -108,7 +118,7 @@ function transformFunction( const rootStatement = getRootStatementPath(path); // Create a unique ID for the function - const fnID = createID(ctx, getDescriptiveName(path, "anonymous")); + const fnID = createID(ctx, path); if (ctx.mode === "server") { // Create a "source" function on the root-level @@ -358,7 +368,7 @@ function transformModuleLevelDirective(ctx: StateContext, program: babel.NodePat return; } - const ids = entries.map(entry => createID(ctx, getDescriptiveName(entry.path, "anonymous"))); + const ids = entries.map(entry => createID(ctx, entry.path)); // clear body program.node.body = [];