From e60891bea2eec17b083b2452fec6e6b1b265521a Mon Sep 17 00:00:00 2001 From: Will Harney <62956339+wjhsf@users.noreply.github.com> Date: Thu, 21 Nov 2024 15:36:35 -0500 Subject: [PATCH] fix(ssr): update wire adapter validation @ W-17274788 (#4910) * test(ssr): add test for wire config object * feat: add wire adapter param validation * test: add some test fixtures for wire * test: add test fixtures for wire * test: refactor tests * feat: validate that the adapter is an imported module * feat: add config validation * feat(ssr): fix wire adapter assumptions * chore(fixtures): standardize output order * chore(ssr): unexpect failures * chore: linter fixes --------- Co-authored-by: James Tu Co-authored-by: Nolan Lawson Co-authored-by: Eugene Kashida --- .../fixtures/wire/config/expected.html | 23 +- .../wire/config/modules/x/wire/adapter.js | 11 +- .../fixtures/wire/field/expected.html | 7 +- .../wire/field/modules/x/adapter/adapter.js | 11 +- .../fixtures/wire/method/expected.html | 7 +- .../wire/method/modules/x/adapter/adapter.js | 11 +- .../src/__tests__/utils/expected-failures.ts | 5 - .../@lwc/ssr-compiler/src/compile-js/index.ts | 4 +- .../@lwc/ssr-compiler/src/compile-js/types.ts | 19 +- .../@lwc/ssr-compiler/src/compile-js/wire.ts | 237 ++++++++++-------- packages/@lwc/ssr-compiler/src/estemplate.ts | 2 +- 11 files changed, 194 insertions(+), 143 deletions(-) diff --git a/packages/@lwc/engine-server/src/__tests__/fixtures/wire/config/expected.html b/packages/@lwc/engine-server/src/__tests__/fixtures/wire/config/expected.html index a584db6ede..73debe9ade 100644 --- a/packages/@lwc/engine-server/src/__tests__/fixtures/wire/config/expected.html +++ b/packages/@lwc/engine-server/src/__tests__/fixtures/wire/config/expected.html @@ -1,23 +1,20 @@ \ No newline at end of file diff --git a/packages/@lwc/engine-server/src/__tests__/fixtures/wire/config/modules/x/wire/adapter.js b/packages/@lwc/engine-server/src/__tests__/fixtures/wire/config/modules/x/wire/adapter.js index 405f2a3765..f96f1a3444 100644 --- a/packages/@lwc/engine-server/src/__tests__/fixtures/wire/config/modules/x/wire/adapter.js +++ b/packages/@lwc/engine-server/src/__tests__/fixtures/wire/config/modules/x/wire/adapter.js @@ -6,8 +6,15 @@ export class adapter { connect() {} update(config) { - // Quotes are encoded in the output, which is ugly - this.dc(JSON.stringify(config, null, 4).replace(/"/g, '')); + // JSON.stringify serializes differently for engine-server/ssr-compiler, so we do it manually + const output = Object.entries(config) + .sort(([a], [b]) => a.localeCompare(b)) + .map(([key, value]) => ` ${key}: ${JSON.stringify(value)},`) + .join('\n') + // Quotes are encoded as " in the output, which makes it harder to read... + .replace(/"/g, ''); + + this.dc(`{\n${output}\n}`); } disconnect() {} diff --git a/packages/@lwc/engine-server/src/__tests__/fixtures/wire/field/expected.html b/packages/@lwc/engine-server/src/__tests__/fixtures/wire/field/expected.html index 218386bc3c..d753c1fdb7 100644 --- a/packages/@lwc/engine-server/src/__tests__/fixtures/wire/field/expected.html +++ b/packages/@lwc/engine-server/src/__tests__/fixtures/wire/field/expected.html @@ -1,11 +1,8 @@ \ No newline at end of file diff --git a/packages/@lwc/engine-server/src/__tests__/fixtures/wire/field/modules/x/adapter/adapter.js b/packages/@lwc/engine-server/src/__tests__/fixtures/wire/field/modules/x/adapter/adapter.js index 2a24199363..6243b8735b 100644 --- a/packages/@lwc/engine-server/src/__tests__/fixtures/wire/field/modules/x/adapter/adapter.js +++ b/packages/@lwc/engine-server/src/__tests__/fixtures/wire/field/modules/x/adapter/adapter.js @@ -6,8 +6,15 @@ export default class adapter { connect() {} update(config) { - // Quotes are encoded in the output, which is ugly - this.dc(JSON.stringify(config, null, 4).replace(/"/g, '')); + // JSON.stringify serializes differently for engine-server/ssr-compiler, so we do it manually + const output = Object.entries(config) + .sort(([a], [b]) => a.localeCompare(b)) + .map(([key, value]) => ` ${key}: ${JSON.stringify(value)},`) + .join('\n') + // Quotes are encoded as " in the output, which makes it harder to read... + .replace(/"/g, ''); + + this.dc(`{\n${output}\n}`); } disconnect() {} diff --git a/packages/@lwc/engine-server/src/__tests__/fixtures/wire/method/expected.html b/packages/@lwc/engine-server/src/__tests__/fixtures/wire/method/expected.html index 218386bc3c..d753c1fdb7 100644 --- a/packages/@lwc/engine-server/src/__tests__/fixtures/wire/method/expected.html +++ b/packages/@lwc/engine-server/src/__tests__/fixtures/wire/method/expected.html @@ -1,11 +1,8 @@ \ No newline at end of file diff --git a/packages/@lwc/engine-server/src/__tests__/fixtures/wire/method/modules/x/adapter/adapter.js b/packages/@lwc/engine-server/src/__tests__/fixtures/wire/method/modules/x/adapter/adapter.js index 2a24199363..6243b8735b 100644 --- a/packages/@lwc/engine-server/src/__tests__/fixtures/wire/method/modules/x/adapter/adapter.js +++ b/packages/@lwc/engine-server/src/__tests__/fixtures/wire/method/modules/x/adapter/adapter.js @@ -6,8 +6,15 @@ export default class adapter { connect() {} update(config) { - // Quotes are encoded in the output, which is ugly - this.dc(JSON.stringify(config, null, 4).replace(/"/g, '')); + // JSON.stringify serializes differently for engine-server/ssr-compiler, so we do it manually + const output = Object.entries(config) + .sort(([a], [b]) => a.localeCompare(b)) + .map(([key, value]) => ` ${key}: ${JSON.stringify(value)},`) + .join('\n') + // Quotes are encoded as " in the output, which makes it harder to read... + .replace(/"/g, ''); + + this.dc(`{\n${output}\n}`); } disconnect() {} diff --git a/packages/@lwc/ssr-compiler/src/__tests__/utils/expected-failures.ts b/packages/@lwc/ssr-compiler/src/__tests__/utils/expected-failures.ts index 0d82e4dd81..04fb37527e 100644 --- a/packages/@lwc/ssr-compiler/src/__tests__/utils/expected-failures.ts +++ b/packages/@lwc/ssr-compiler/src/__tests__/utils/expected-failures.ts @@ -51,9 +51,4 @@ export const expectedFailures = new Set([ 'superclass/render-in-superclass/unused-default-in-subclass/index.js', 'superclass/render-in-superclass/unused-default-in-superclass/index.js', 'svgs/index.js', - 'wire/config/index.js', - 'wire/deep-reference/index.js', - 'wire/field/index.js', - 'wire/imported-member/index.js', - 'wire/method/index.js', ]); diff --git a/packages/@lwc/ssr-compiler/src/compile-js/index.ts b/packages/@lwc/ssr-compiler/src/compile-js/index.ts index e5ed74c0bf..5d7b25c8f1 100644 --- a/packages/@lwc/ssr-compiler/src/compile-js/index.ts +++ b/packages/@lwc/ssr-compiler/src/compile-js/index.ts @@ -74,7 +74,7 @@ const visitors: Visitors = { is.identifier(decoratedExpression.callee) && decoratedExpression.callee.name === 'wire' ) { - catalogWireAdapters(state, node); + catalogWireAdapters(path, state); state.privateFields.push(node.key.name); } else { state.privateFields.push(node.key.name); @@ -111,7 +111,7 @@ const visitors: Visitors = { is.identifier(decoratedExpression.callee) && decoratedExpression.callee.name === 'wire' ) { - catalogWireAdapters(state, node); + catalogWireAdapters(path, state); } switch (node.key.name) { diff --git a/packages/@lwc/ssr-compiler/src/compile-js/types.ts b/packages/@lwc/ssr-compiler/src/compile-js/types.ts index 81412f60a3..335fc0e2f3 100644 --- a/packages/@lwc/ssr-compiler/src/compile-js/types.ts +++ b/packages/@lwc/ssr-compiler/src/compile-js/types.ts @@ -6,18 +6,21 @@ */ import type { traverse } from 'estree-toolkit'; -import type { Node } from 'estree'; +import type { + Identifier, + MemberExpression, + MethodDefinition, + Node, + ObjectExpression, + PropertyDefinition, +} from 'estree'; export type Visitors = Parameters>[1]; export interface WireAdapter { - fieldName: string; - adapterConstructorId: string; - config: Array<{ - configKey: string; - referencedField: string; - }>; - fieldType: 'property' | 'method'; + adapterId: Identifier | MemberExpression; + config: ObjectExpression; + field: MethodDefinition | PropertyDefinition; } export interface ComponentMetaState { diff --git a/packages/@lwc/ssr-compiler/src/compile-js/wire.ts b/packages/@lwc/ssr-compiler/src/compile-js/wire.ts index dabd55848d..1ed5b5fc12 100644 --- a/packages/@lwc/ssr-compiler/src/compile-js/wire.ts +++ b/packages/@lwc/ssr-compiler/src/compile-js/wire.ts @@ -6,109 +6,157 @@ */ import { is, builders as b } from 'estree-toolkit'; +import { produce } from 'immer'; import { esTemplate } from '../estemplate'; -import { isValidIdentifier } from '../shared'; +import type { NodePath } from 'estree-toolkit'; -import type { PropertyDefinition as EsPropertyDefinitionWithDecorators } from 'meriyah/dist/src/estree'; import type { - Expression, PropertyDefinition, ObjectExpression, - Statement, MethodDefinition, ExpressionStatement, + Expression, + Identifier, + MemberExpression, + Property, + BlockStatement, } from 'estree'; import type { ComponentMetaState, WireAdapter } from './types'; -function extractWireConfig( - fieldName: string, - adapterConstructorId: string, - fieldType: 'method' | 'property', - config?: ObjectExpression -): WireAdapter { - const extractedConfig = - config?.properties?.map?.((objProp) => { - if (!is.property(objProp)) { - throw new Error('Object spread syntax is disallowed in @wire config.'); - } - const { key, value } = objProp; - - if (!is.identifier(key)) { - throw new Error(`@wire config entry key must be an identifer; found ${key.type}`); - } - if (!is.literal(value) || typeof value.value !== 'string' || value.value[0] !== '$') { - throw new Error('@wire config entry values must be strings starting with a $'); - } - const referencedField = value.value.slice(1); - if (!isValidIdentifier(referencedField)) { - throw new Error(`@wire config referenced invalid field: ${referencedField}`); - } +interface NoSpreadObjectExpression extends Omit { + properties: Property[]; +} - return { - configKey: key.name, - referencedField, - }; - }) ?? []; - - return { - fieldName, - fieldType, - adapterConstructorId, - config: extractedConfig, - }; +function bMemberExpressionChain(props: string[]): MemberExpression { + // Technically an incorrect assertion, but it works fine... + let expr: MemberExpression = b.identifier('instance') as any; + for (const prop of props) { + expr = b.memberExpression(expr, b.literal(prop), true); + } + return expr; } -export function catalogWireAdapters( - state: ComponentMetaState, - node: PropertyDefinition | MethodDefinition -) { - if (!is.identifier(node.key)) { - throw new Error( - 'Unimplemented: wires that decorate non-identifiers are not currently supported.' - ); +function getWireParams( + node: MethodDefinition | PropertyDefinition +): [Expression, Expression | undefined] { + const { decorators } = node; + + if (decorators.length > 1) { + throw new Error('todo - multiple decorators at once'); + } + + // validate the parameters + const wireDecorator = decorators[0].expression; + if (!is.callExpression(wireDecorator)) { + throw new Error('todo - invalid usage'); + } + + const args = wireDecorator.arguments; + if (args.length === 0 || args.length > 2) { + throw new Error('todo - wrong number of args'); } - const { name } = node.key; - const { decorators } = node as EsPropertyDefinitionWithDecorators; - if (decorators?.length !== 1) { - throw new Error('Only one decorator can be applied to a single field.'); + const [id, config] = args; + if (is.spreadElement(id) || is.spreadElement(config)) { + throw new Error('todo - spread in params'); + } + return [id, config]; +} + +function validateWireId( + id: Expression, + path: NodePath +): asserts id is Identifier | MemberExpression { + // name of identifier or object used in member expression (e.g. "foo" for `foo.bar`) + let wireAdapterVar: string; + + if (is.memberExpression(id)) { + if (id.computed) { + throw new Error('todo - FUNCTION_IDENTIFIER_CANNOT_HAVE_COMPUTED_PROPS'); + } + if (!is.identifier(id.object)) { + throw new Error('todo - FUNCTION_IDENTIFIER_CANNOT_HAVE_NESTED_MEMBER_EXRESSIONS'); + } + wireAdapterVar = id.object.name; + } else if (!is.identifier(id)) { + throw new Error('todo - invalid adapter name'); + } else { + wireAdapterVar = id.name; } - const expression = decorators[0].expression as Expression; - if (!is.callExpression(expression)) { - throw new Error('The @wire decorator must be called.'); + // This is not the exact same validation done in @lwc/babel-plugin-component but it accomplishes the same thing + if (path.scope?.getBinding(wireAdapterVar)?.kind !== 'module') { + throw new Error('todo - WIRE_ADAPTER_SHOULD_BE_IMPORTED'); } +} - const { arguments: args } = expression; - const [adapterConstructorId, config] = args; - if (!is.identifier(adapterConstructorId)) { - throw new Error('The @wire decorator must reference a wire adapter class.'); +function validateWireConfig( + config: Expression, + path: NodePath +): asserts config is NoSpreadObjectExpression { + if (!is.objectExpression(config)) { + throw new Error('todo - CONFIG_OBJECT_SHOULD_BE_SECOND_PARAMETER'); } - if (config && !is.objectExpression(config)) { - throw new Error('Invalid config provided to @wire decorator; expected an object literal.'); + for (const property of config.properties) { + // Only validate computed object properties because static props are all valid + // and we ignore {...spreads} and {methods(){}} + if (!is.property(property) || !property.computed) continue; + const key = property.key; + if (is.identifier(key)) { + const binding = path.scope?.getBinding(key.name); + // TODO [#3956]: Investigate allowing imported constants + if (binding?.kind === 'const') continue; + // By default, the identifier `undefined` has no binding (when it's actually undefined), + // but has a binding if it's used as a variable (e.g. `let undefined = "don't do this"`) + if (key.name === 'undefined' && !binding) continue; + } else if (is.literal(key)) { + if (is.templateLiteral(key)) { + // A template literal is not guaranteed to always result in the same value + // (e.g. `${Math.random()}`), so we disallow them entirely. + throw new Error('todo - COMPUTED_PROPERTY_CANNOT_BE_TEMPLATE_LITERAL'); + } else if (!('regex' in key)) { + // A literal can be a regexp, template literal, or primitive; only allow primitives + continue; + } + } + throw new Error('todo - COMPUTED_PROPERTY_MUST_BE_CONSTANT_OR_LITERAL'); } +} - const fieldtype = - node.type === 'MethodDefinition' && node.kind === 'method' ? 'method' : 'property'; +export function catalogWireAdapters( + path: NodePath, + state: ComponentMetaState +) { + const node = path.node!; + const [id, config] = getWireParams(node); + validateWireId(id, path); + let reactiveConfig: ObjectExpression; + if (config) { + validateWireConfig(config, path); + reactiveConfig = produce(config, (draft) => { + // replace '$foo' values with `instance.foo`; preserve everything else + for (const prop of draft.properties) { + const { value } = prop; + if ( + is.literal(value) && + typeof value.value === 'string' && + value.value.startsWith('$') + ) { + prop.value = bMemberExpressionChain(value.value.slice(1).split('.')); + } + } + }); + } else { + // FIXME: for `@wire(Adapter), does engine-server use `undefined` or `{}` for config? + reactiveConfig = b.objectExpression([]); // empty object + } state.wireAdapters = [ ...state.wireAdapters, - extractWireConfig(name, adapterConstructorId.name, fieldtype, config), + { adapterId: id, config: reactiveConfig, field: node }, ]; } -function bWireConfigObj(adapter: WireAdapter) { - return b.objectExpression( - adapter.config.map(({ configKey, referencedField }) => - b.property( - 'init', - b.identifier(configKey), - b.memberExpression(b.identifier('instance'), b.identifier(referencedField)) - ) - ) - ); -} - const bSetWiredProp = esTemplate` instance.${/*wire-decorated property*/ is.identifier} = newValue `; @@ -117,41 +165,34 @@ const bCallWiredMethod = esTemplate` instance.${/*wire-decorated method*/ is.identifier}(newValue) `; -const bWireAdapterPlumbing = esTemplate` - const wireInstance = new ${/*wire adapter constructor*/ is.identifier}((newValue) => { +const bWireAdapterPlumbing = esTemplate`{ + const wireInstance = new ${/*wire adapter constructor*/ is.expression}((newValue) => { ${/*update the decorated property or call the decorated method*/ is.expressionStatement}; }); wireInstance.connect?.(); if (wireInstance.update) { - const wireConfigObj = ${/*mapping from lwc fields to wire config keys*/ is.objectExpression}; - + const getLiveConfig = () => { + return ${/* reactive wire config */ is.objectExpression}; + }; // This may look a bit weird, in that the 'update' function is called twice: once with // an 'undefined' value and possibly again with a context-provided value. While weird, // this preserves the behavior of the browser-side wire implementation as well as the // original SSR implementation. - wireInstance.update(wireConfigObj, undefined); + wireInstance.update(getLiveConfig(), undefined); __connectContext(${/*wire adapter constructor*/ 0}, instance, (newContextValue) => { - const wireConfigObj = ${/*mapping from lwc fields to wire config keys*/ 2}; - wireInstance.update(wireConfigObj, newContextValue); + wireInstance.update(getLiveConfig(), newContextValue); }); } -`; - -export function bWireAdaptersPlumbing(adapters: WireAdapter[]): Statement[] { - return adapters.map((adapter) => { - const { adapterConstructorId, fieldName } = adapter; +}`; +export function bWireAdaptersPlumbing(adapters: WireAdapter[]): BlockStatement[] { + return adapters.map(({ adapterId, config, field }) => { const actionUponNewValue = - adapter.fieldType === 'method' - ? bCallWiredMethod(b.identifier(fieldName)) - : bSetWiredProp(b.identifier(fieldName)); - - return b.blockStatement( - bWireAdapterPlumbing( - b.identifier(adapterConstructorId), - actionUponNewValue, - bWireConfigObj(adapter) - ) - ); + is.methodDefinition(field) && field.kind === 'method' + ? // Validation in compile-js/index.ts `visitors` ensures `key` is an identifier + bCallWiredMethod(field.key as Identifier) + : bSetWiredProp(field.key as Identifier); + + return bWireAdapterPlumbing(adapterId, actionUponNewValue, config); }); } diff --git a/packages/@lwc/ssr-compiler/src/estemplate.ts b/packages/@lwc/ssr-compiler/src/estemplate.ts index a655aa26b1..6df0148fa0 100644 --- a/packages/@lwc/ssr-compiler/src/estemplate.ts +++ b/packages/@lwc/ssr-compiler/src/estemplate.ts @@ -97,7 +97,7 @@ const getReplacementNode = ( validateReplacement.name || '(could not determine)'; const actualType = Array.isArray(replacementNode) - ? `[${replacementNode.map((n) => n && n.type)}.join(', ')]` + ? `[${replacementNode.map((n) => n && n.type).join(', ')}]` : replacementNode?.type; throw new Error( `Validation failed for templated node. Expected type ${expectedType}, but received ${actualType}.`