also looking at this
fix(ssr): respect client signal ownership before evaluation
#2100
2 files
+46
-29
Review threads live on the whole diff, not on one commit, so none are shown here - a thread's line means something in the branch's final form, and painting it into an intermediate step would put it on code it is not about.
| @@ -35,7 +35,7 @@ import fs from 'node:fs' | ||
| 35 | 35 | import { isDevelopment } from './env' |
| 36 | 36 | import { stripCommentsAndLiterals } from './strip-literals' |
| 37 | 37 | import { createSafeFunction, getExpressionSafetyRule, safeEvaluate } from './safe-evaluator' |
| 38 | import { createDetailedErrorMessage } from './utils' | |
| 38 | import { componentClientSignalNames, createDetailedErrorMessage } from './utils' | |
| 39 | 39 | import { createPlaceholder } from './placeholder' |
| 40 | 40 | import { neutralizeTemplateSyntax } from './template-syntax-escape' |
| 41 | 41 | import { classifyHtmlInterpolation, UnsafeInterpolationContextError } from './interpolation-context' |
| @@ -717,15 +717,19 @@ function escapeInterpolatedHtmlValue( | ||
| 717 | 717 | * recognised too. Without it the scan is text-only, which is what every caller |
| 718 | 718 | * did before and is still correct for a script that declares its own signals. |
| 719 | 719 | */ |
| 720 | /** Bodies of inline client scripts already found at genuine element positions. */ | |
| 721 | function clientScriptBodies(scripts: string[]): string[] { | |
| 722 | return scripts.flatMap((script) => { | |
| 723 | const match = script.match(/^<script\b([^>]*)>([\s\S]*?)<\/script\s*>$/i) | |
| 724 | return !match || /\bserver\b|\bsrc\s*=/.test(match[1]) ? [] : [match[2]] | |
| 725 | }) | |
| 726 | } | |
| 727 | ||
| 720 | 728 | export function usesSignalsInScript(template: string, filePath?: string): boolean { |
| 721 | 729 | // Check genuine script elements only. Tag-like text inside comments, |
| 722 | 730 | // attributes, or style bodies is not executable script. |
| 723 | 731 | const { scripts } = stashScriptElements(template) |
| 724 | for (const script of scripts) { | |
| 725 | const match = script.match(/^<script\b([^>]*)>([\s\S]*?)<\/script\s*>$/i) | |
| 726 | if (!match || /\bserver\b|\bsrc\s*=/.test(match[1])) | |
| 727 | continue | |
| 728 | const content = match[2] | |
| 732 | for (const content of clientScriptBodies(scripts)) { | |
| 729 | 733 | if (/\b(state|derived|effect|ref|reactive|computed|watch|watchEffect|useModel)\s*(?:<[^>]*>)?\s*\(/.test(content)) { |
| 730 | 734 | return true |
| 731 | 735 | } |
| @@ -1216,11 +1220,10 @@ export function processExpressions(template: string, context: Record<string, any | ||
| 1216 | 1220 | // static prop shadows the signal. The Set stays. |
| 1217 | 1221 | const hasSignals = context?.__stx_signals_gate === true |
| 1218 | 1222 | || usesSignalsInScript(template) |
| 1219 | const clientSignalNames = new Set<string>( | |
| 1220 | Array.isArray(context?.__stx_client_signal_names) | |
| 1221 | ? context.__stx_client_signal_names | |
| 1222 | : [], | |
| 1223 | ) | |
| 1223 | const clientSignalNames = new Set<string>([ | |
| 1224 | ...(Array.isArray(context?.__stx_client_signal_names) ? context.__stx_client_signal_names : []), | |
| 1225 | ...componentClientSignalNames(clientScriptBodies(scriptBlocks)), | |
| 1226 | ]) | |
| 1224 | 1227 | const preservedClientExpressions = new Set<string>( |
| 1225 | 1228 | Array.isArray(context?.__stx_preserved_client_expressions) |
| 1226 | 1229 | ? context.__stx_preserved_client_expressions |
| @@ -1345,6 +1348,14 @@ export function processExpressions(template: string, context: Record<string, any | ||
| 1345 | 1348 | return '' |
| 1346 | 1349 | } |
| 1347 | 1350 | |
| 1351 | // An authored client signal owns its binding before evaluation. A server | |
| 1352 | // prop or auto-import can have the same name (e.g. the POST helper `post`); | |
| 1353 | // calling that helper first can submit a request even when its result is | |
| 1354 | // discarded. Component scripts have already been lifted out, so inherit | |
| 1355 | // their recorded names alongside declarations in this page's own scripts. | |
| 1356 | if (firstVarIsClientSignal) | |
| 1357 | return match | |
| 1358 | ||
| 1348 | 1359 | // Try evaluation first - this allows user-defined helper functions to work |
| 1349 | 1360 | // Only preserve for client-side if evaluation fails AND it looks like a signal |
| 1350 | 1361 | let rejection: ExpressionRejection | undefined |
| @@ -1353,20 +1364,6 @@ export function processExpressions(template: string, context: Record<string, any | ||
| 1353 | 1364 | rejection = r |
| 1354 | 1365 | }) |
| 1355 | 1366 | |
| 1356 | // A component prop can have a static server value and then be shadowed | |
| 1357 | // by a client signal with the same name: | |
| 1358 | // | |
| 1359 | // const title = useReactiveProp('title', '') | |
| 1360 | // <h2></h2> | |
| 1361 | // | |
| 1362 | // The server context contains `title: "Hello"`, so context-only | |
| 1363 | // detection cannot distinguish this from a server helper and used to | |
| 1364 | // evaluate the expression away. The component renderer records locally | |
| 1365 | // declared client signals before stripping its script, allowing the | |
| 1366 | // interpolation to survive for the scoped runtime. | |
| 1367 | if (firstVarIsClientSignal) | |
| 1368 | return match | |
| 1369 | ||
| 1370 | 1367 | // For signal-based templates: if the expression references variables NOT in the |
| 1371 | 1368 | // server-side context (i.e. client-only signals), preserve it for client-side |
| 1372 | 1369 | // processing REGARDLESS of the server-evaluated value. Gating on `value === undefined` |
| @@ -1422,9 +1419,6 @@ export function processExpressions(template: string, context: Record<string, any | ||
| 1422 | 1419 | catch (error: unknown) { |
| 1423 | 1420 | if (error instanceof UnsafeInterpolationContextError) |
| 1424 | 1421 | throw error |
| 1425 | if (firstVarIsClientSignal) | |
| 1426 | return match | |
| 1427 | ||
| 1428 | 1422 | // For signal-based templates, preserve expressions that reference runtime variables |
| 1429 | 1423 | // This includes loop variables (page, event, etc.) and component-defined functions (fmt, etc.) |
| 1430 | 1424 | if (hasSignals && !expressionUsesOnlyContextVars(trimmedExpr, context)) { |
| @@ -27,7 +27,7 @@ | ||
| 27 | 27 | * probed already preserved correctly. These tests therefore pin the stamp's own |
| 28 | 28 | * contract, which is what the next steps will build on. |
| 29 | 29 | */ |
| 30 | import { afterAll, beforeAll, describe, expect, it } from 'bun:test' | |
| 30 | import { afterAll, beforeAll, describe, expect, it, mock } from 'bun:test' | |
| 31 | 31 | import fs from 'node:fs' |
| 32 | 32 | import os from 'node:os' |
| 33 | 33 | import path from 'node:path' |
| @@ -54,6 +54,10 @@ const label = derived(() => 'live') | ||
| 54 | 54 | fs.writeFileSync(path.join(dir, 'partials', 'plain.stx'), '<span></span>') |
| 55 | 55 | fs.mkdirSync(path.join(dir, 'components'), { recursive: true }) |
| 56 | 56 | fs.writeFileSync(path.join(dir, 'components', 'Alert.stx'), '<div class="alert"><slot /></div>') |
| 57 | fs.writeFileSync(path.join(dir, 'components', 'PostEditor.stx'), `<script client> | |
| 58 | const post = useReactiveProp('post', null) | |
| 59 | </script> | |
| 60 | <button>{{ post() ? 'Save changes' : 'Create post' }}</button>`) | |
| 57 | 61 | }) |
| 58 | 62 | |
| 59 | 63 | afterAll(() => { |
| @@ -136,6 +140,25 @@ const n = state(0) | ||
| 136 | 140 | * "untested". These are those tests. |
| 137 | 141 | */ |
| 138 | 142 | describe('cases the per-caller overrides used to carry', () => { |
| 143 | it.each([ | |
| 144 | `<script client>const post = state(null)</script><button>Save changes</button>`, | |
| 145 | '<PostEditor />', | |
| 146 | ])('never invokes a server helper shadowed by an authored client signal', async (template) => { | |
| 147 | // Stacks auto-imports its POST helper as `post`. Evaluating the client | |
| 148 | // editor's signal before preserving it used to submit an undefined URL. | |
| 149 | const post = mock(() => Promise.resolve({ server: true })) | |
| 150 | const out = await processDirectives(template, { post }, path.join(dir, 'page.stx'), options(), new Set<string>()) | |
| 151 | expect(post).not.toHaveBeenCalled() | |
| 152 | expect(out).toContain("Save changes") | |
| 153 | }) | |
| 154 | ||
| 155 | it('still evaluates a server helper when no client signal owns its name', async () => { | |
| 156 | const title = mock(() => 'Server title') | |
| 157 | const out = await processDirectives('<h1></h1>', { title }, path.join(dir, 'page.stx'), options(), new Set<string>()) | |
| 158 | expect(out).toContain('<h1>Server title</h1>') | |
| 159 | expect(title).toHaveBeenCalledTimes(1) | |
| 160 | }) | |
| 161 | ||
| 139 | 162 | it('preserves slot content authored by the CALLER', async () => { |
| 140 | 163 | // options.__stx_force_signals existed for this: `<Alert></Alert>` |
| 141 | 164 | // is written by the caller, so judging it by the COMPONENT's own script |