From b485b976e11cf4f79a5c2564b599bbef8a0a4d8a Mon Sep 17 00:00:00 2001 From: Titouan Mathis Date: Wed, 19 Aug 2026 14:18:15 +0200 Subject: [PATCH] refactor(v4): converge the two expression evaluators Action/expression.ts and Data/expression.ts each had their own new Function cache with their own key scheme, plus an uncached third call site in Fetch.parseResponse() that recompiled on every response. Data's key = code + name also let two unrelated expressions collide, e.g. ('bc', 'return "A"') and ('c', 'return "A"b'). Replace all three with one compileExpression(argNames, body), cached in two Map levels instead of a hand-built cacheKey string so nothing a caller writes can collide. Data's getCallback() drops its group parameter, which was never part of the executed function's arguments. Closes F2 in #780. --- packages/v4/migration/Action/expression.ts | 31 ++-------------- packages/v4/migration/Data/DataBind.ts | 6 +--- packages/v4/migration/Data/DataComputed.ts | 3 +- packages/v4/migration/Data/DataEffect.ts | 3 +- packages/v4/migration/Data/expression.ts | 27 +++++--------- packages/v4/migration/Fetch/Fetch.ts | 19 +++++----- packages/v4/migration/expression.spec.ts | 24 +++++++++++++ packages/v4/migration/expression.ts | 42 ++++++++++++++++++++++ packages/v4/migration/index.ts | 3 ++ 9 files changed, 94 insertions(+), 64 deletions(-) create mode 100644 packages/v4/migration/expression.spec.ts create mode 100644 packages/v4/migration/expression.ts diff --git a/packages/v4/migration/Action/expression.ts b/packages/v4/migration/Action/expression.ts index 907b7c3a0..d91e05102 100644 --- a/packages/v4/migration/Action/expression.ts +++ b/packages/v4/migration/Action/expression.ts @@ -1,30 +1,11 @@ -/** - * Expression evaluator for action bindings. It uses `new Function`, so it - * requires a Content Security Policy that permits `unsafe-eval`. - */ +import { compileExpression, type CompiledExpression } from '../expression.js'; /** * A compiled effect. Called with `this` bound to the action's element and the * argument list `executeEffect()` assembles; may return a function, which is * then called with the same `this` and the same arguments. */ -export type EffectFunction = (...args: unknown[]) => unknown; - -const cache = new Map(); - -/** - * Compile `body` into a function taking `argNames`, memoised under `cacheKey`. - */ -function compile(argNames: readonly string[], body: string, cacheKey: string): EffectFunction { - let callback = cache.get(cacheKey); - if (!callback) { - // Compiling the author's expression is the whole feature. - // oxlint-disable-next-line no-new-func, typescript/no-implied-eval - callback = new Function(...argNames, body) as EffectFunction; - cache.set(cacheKey, callback); - } - return callback; -} +export type EffectFunction = CompiledExpression; /** Effect argument names in `executeEffect()` order. */ export const EFFECT_ARGUMENTS = ['ctx', 'event', 'target', 'action', 'self', '$el'] as const; @@ -32,15 +13,9 @@ export const EFFECT_ARGUMENTS = ['ctx', 'event', 'target', 'action', 'self', '$e /** * Compile one `data-on:*` or `data-option-effect` expression. * - * The cache key includes the source and complete parameter list. - * * @param code The expression source, for example `target.open()`. * @param instanceNames Co-located component names added as parameters. */ export function getEffect(code: string, instanceNames: readonly string[]): EffectFunction { - return compile( - [...EFFECT_ARGUMENTS, ...instanceNames], - `return ${code}`, - `${instanceNames.length}:${instanceNames.join(',')}:${code}`, - ); + return compileExpression([...EFFECT_ARGUMENTS, ...instanceNames], `return ${code}`); } diff --git a/packages/v4/migration/Data/DataBind.ts b/packages/v4/migration/Data/DataBind.ts index 137c31627..1ff11d739 100644 --- a/packages/v4/migration/Data/DataBind.ts +++ b/packages/v4/migration/Data/DataBind.ts @@ -325,11 +325,7 @@ export class DataBind if (binding.expression) { try { - result = getCallback(this.group, `return ${binding.expression};`)( - value, - this.target, - this.$data, - ); + result = getCallback(`return ${binding.expression};`)(value, this.target, this.$data); } catch (error) { console.error('[data] Binding expression failed:', error); continue; diff --git a/packages/v4/migration/Data/DataComputed.ts b/packages/v4/migration/Data/DataComputed.ts index f5c49cac9..aa71ecf9e 100644 --- a/packages/v4/migration/Data/DataComputed.ts +++ b/packages/v4/migration/Data/DataComputed.ts @@ -22,8 +22,7 @@ export class DataComputed extends DataBind { } get compute(): DataExpression { - const { group, compute } = this.$options; - return getCallback(group, `return ${compute};`); + return getCallback(`return ${this.$options.compute};`); } override set(value: DataValue): void { diff --git a/packages/v4/migration/Data/DataEffect.ts b/packages/v4/migration/Data/DataEffect.ts index cbc0fe5d2..da72fcedf 100644 --- a/packages/v4/migration/Data/DataEffect.ts +++ b/packages/v4/migration/Data/DataEffect.ts @@ -22,8 +22,7 @@ export class DataEffect extends DataBind { } get effect(): DataExpression { - const { group, effect } = this.$options; - return getCallback(group, effect); + return getCallback(this.$options.effect); } override set(value: DataValue): void { diff --git a/packages/v4/migration/Data/expression.ts b/packages/v4/migration/Data/expression.ts index 77c30b9ae..c5ffb813b 100644 --- a/packages/v4/migration/Data/expression.ts +++ b/packages/v4/migration/Data/expression.ts @@ -1,23 +1,14 @@ -/** - * Expression evaluator for Data components. It uses `new Function`, so it - * requires a Content Security Policy that permits `unsafe-eval`. The cache - * keeps identical expressions in different groups separate. - */ +import { compileExpression } from '../expression.js'; export type DataExpression = (value: unknown, target: HTMLElement, $data: unknown) => unknown; -const callbacks = new Map(); - -export function getCallback(name: string, code: string): DataExpression { - const key = code + name; +const ARG_NAMES = ['value', 'target', '$data'] as const; - let callback = callbacks.get(key); - if (!callback) { - // Compiling the author's expression is the whole feature. - // oxlint-disable-next-line no-new-func, typescript/no-implied-eval - callback = new Function('value', 'target', '$data', code) as DataExpression; - callbacks.set(key, callback); - } - - return callback; +/** + * Compile a `data-bind:*`, `data-compute` or `data-effect` expression. + * + * @param code The expression source, for example `return value * 2;`. + */ +export function getCallback(code: string): DataExpression { + return compileExpression(ARG_NAMES, code); } diff --git a/packages/v4/migration/Fetch/Fetch.ts b/packages/v4/migration/Fetch/Fetch.ts index f02f89a4c..3e7bb3c72 100644 --- a/packages/v4/migration/Fetch/Fetch.ts +++ b/packages/v4/migration/Fetch/Fetch.ts @@ -12,6 +12,7 @@ import { type SwapMode, } from '../../src/index.js'; import { historyPush } from '../../src/utils/history.js'; +import { compileExpression } from '../expression.js'; /** * The lifecycle events a `Fetch` announces. @@ -46,6 +47,9 @@ export const HEADER_NAMES = Object.freeze({ /** One parser for every instance: it holds no state between calls. */ const domParser = new DOMParser(); +/** `response` expression argument names, in `parseResponse()`'s call order. */ +const RESPONSE_ARGUMENTS = ['response', 'url', 'requestInit', 'self'] as const; + /** The context every lifecycle event carries. */ export interface FetchEventBase { instance: Fetch; @@ -351,15 +355,12 @@ export class Fetch extends Base * @protected */ parseResponse(response: Response, url: URL, requestInit: RequestInit): Promise | string { - // Evaluating the author's `response` expression is the documented feature. - // oxlint-disable-next-line no-new-func, typescript/no-implied-eval - const fn = new Function( - 'response', - 'url', - 'requestInit', - 'self', - `return ${this.$options.response}`, - ) as (response: Response, url: URL, requestInit: RequestInit, self: unknown) => string; + const fn = compileExpression(RESPONSE_ARGUMENTS, `return ${this.$options.response}`) as ( + response: Response, + url: URL, + requestInit: RequestInit, + self: unknown, + ) => string; return fn.call(this, response, url, requestInit, self); } diff --git a/packages/v4/migration/expression.spec.ts b/packages/v4/migration/expression.spec.ts new file mode 100644 index 000000000..99cf5ec2d --- /dev/null +++ b/packages/v4/migration/expression.spec.ts @@ -0,0 +1,24 @@ +import { describe, expect, it } from 'vitest'; +import { compileExpression } from './expression.js'; + +describe('compileExpression', () => { + it('does not collide two argument lists split at different boundaries', () => { + // `Action`'s v3 predecessor keyed its cache by `effectDefinition + + // keys.join('')`, under which `['Ab', 'C']` and `['A', 'bC']` land in the + // same entry: joined without a separator, both produce `AbC`. A body that + // returns its first argument must resolve `Ab` and `A` to their own value, + // not to whichever list compiled first. + const first = compileExpression(['Ab', 'C'], 'return Ab;'); + const second = compileExpression(['A', 'bC'], 'return A;'); + + expect(first('one', 'two')).toBe('one'); + expect(second('three', 'four')).toBe('three'); + }); + + it('reuses the compiled function for the same argument names and body', () => { + const first = compileExpression(['x'], 'return x;'); + const second = compileExpression(['x'], 'return x;'); + + expect(first).toBe(second); + }); +}); diff --git a/packages/v4/migration/expression.ts b/packages/v4/migration/expression.ts new file mode 100644 index 000000000..f92c8df9e --- /dev/null +++ b/packages/v4/migration/expression.ts @@ -0,0 +1,42 @@ +/** + * The one `new Function` compiler, shared by every family that evaluates an + * author's expression — `Action`, `Data` and `Fetch`. It uses `new Function`, + * so it requires a Content Security Policy that permits `unsafe-eval`; that + * is why the boundary is one file, not a helper each family reinvents. + * + * `Action` and `Data` used to cache under a `cacheKey` the caller built by + * hand — `Data`'s was `code + name`, under which `('bc', 'return "A"')` and + * `('c', 'return "A"b')` land in the same entry. There is no `cacheKey` + * parameter here: the key is the argument list and the body, kept apart in + * two map levels rather than joined into one string, so nothing a caller + * writes can collide with it. + */ + +export type CompiledExpression = (...args: unknown[]) => unknown; + +const cache = new Map>(); + +/** + * Compile `body` into a function taking `argNames`, memoised by both. + * + * @param argNames The parameter names `body` may reference, in order. + * @param body The function body, for example `return target.open();`. + */ +export function compileExpression(argNames: readonly string[], body: string): CompiledExpression { + const argsKey = argNames.join(','); + let byBody = cache.get(argsKey); + if (!byBody) { + byBody = new Map(); + cache.set(argsKey, byBody); + } + + let compiled = byBody.get(body); + if (!compiled) { + // Compiling the author's expression is the whole feature. + // oxlint-disable-next-line no-new-func, typescript/no-implied-eval + compiled = new Function(...argNames, body) as CompiledExpression; + byBody.set(body, compiled); + } + + return compiled; +} diff --git a/packages/v4/migration/index.ts b/packages/v4/migration/index.ts index 181adb409..ffbc5b2f3 100644 --- a/packages/v4/migration/index.ts +++ b/packages/v4/migration/index.ts @@ -19,3 +19,6 @@ export * from './Transition/index.js'; /** Shared vocabulary, not a family: what the parts of a declaration mean. */ export * from './event-modifiers.js'; + +/** Shared primitive, not a family: the one `new Function` compiler. */ +export * from './expression.js';