Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(ssr-compiler): validate api decorator usage #5071

Draft
wants to merge 18 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
168 changes: 168 additions & 0 deletions packages/@lwc/ssr-compiler/src/__tests__/api-decorator.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,168 @@
import { describe, test, expect } from 'vitest';
import { compileComponentForSSR } from '../index';

const compile =
(src: string, filename = 'test.js') =>
() => {
return compileComponentForSSR(src, filename, {});
};

describe('thows error', () => {
test('combined with @track', () => {
const src = /* js */ `
import { api, track, LightningElement } from "lwc";
export default class Test extends LightningElement {
@track
@api
apiWithTrack = "foo";
}
`;
expect(compile(src)).toThrow(`LWC1093: @api method or property cannot be used with @track`);
});
cardoso marked this conversation as resolved.
Show resolved Hide resolved

describe('conflicting api properties', () => {
test.for([
[
'getter/setter',
/* js */ `
@api foo = 1;
_internal = 1;
@api
get foo() {
return "foo";
}
set foo(val) {
this._internal = val;
}`,
],
[
'method',
/* js */ `
@api foo = 1;
@api foo() {
return "foo";
}`,
],
] as [name: string, body: string][])(`%s`, ([, body]) => {
const src = /* js */ `
import { api, LightningElement } from "lwc";
export default class Test extends LightningElement {
${body}
}
`;
expect(compile(src)).toThrow(`LWC1096: Duplicate @api property "foo".`);
});
});

test('default value is true', () => {
const src = /* js */ `
import { api, LightningElement } from "lwc";
export default class Test extends LightningElement {
@api publicProp = true;
}
`;
expect(compile(src)).toThrow(`LWC1099: Boolean public property must default to false.`);
});

test('computed api getters and setters', () => {
const src = /* js */ `
import { api, LightningElement } from "lwc";
export default class Test extends LightningElement {
@api
set [x](value) {}
get [x]() {}
}
`;
expect(compile(src)).toThrow(
`LWC1106: @api cannot be applied to a computed property, getter, setter or method.`
);
});

test('property name prefixed with data', () => {
const src = /* js */ `
import { api, LightningElement } from "lwc";
export default class Test extends LightningElement {
@api dataFooBar;
}
`;
expect(compile(src)).toThrow(
`LWC1107: Invalid property name "dataFooBar". Properties starting with "data" are reserved attributes.`
);
});

test('property name prefixed with on', () => {
const src = /* js */ `
import { api, LightningElement } from "lwc";
export default class Test extends LightningElement {
@api onChangeHandler;
}
`;
expect(compile(src)).toThrow(
`LWC1108: Invalid property name "onChangeHandler". Properties starting with "on" are reserved for event handlers`
);
});

describe('property name is ambiguous', () => {
test.for([
['bgcolor', 'bgColor'],
['accesskey', 'accessKey'],
['contenteditable', 'contentEditable'],
['tabindex', 'tabIndex'],
['maxlength', 'maxLength'],
['maxvalue', 'maxValue'],
] as [prop: string, suggestion: string][])('%s', ([prop, suggestion]) => {
const src = /* js */ `
import { api, LightningElement } from "lwc";
export default class Test extends LightningElement {
@api ${prop};
}
`;
expect(compile(src)).toThrow(
`LWC1109: Ambiguous attribute name "${prop}". "${prop}" will never be called from template because its corresponding property is camel cased. Consider renaming to "${suggestion}"`
);
});
});

describe('disallowed props', () => {
test.for(['class', 'is', 'slot', 'style'])('%s', (prop) => {
const src = /* js */ `
import { api, LightningElement } from 'lwc'
export default class Test extends LightningElement {
@api ${prop}
}
`;
expect(compile(src)).toThrow(
`LWC1110: Invalid property name "${prop}". "${prop}" is a reserved attribute.`
);
});
});

test('property name is part', () => {
const src = /* js */ `
import { api, LightningElement } from "lwc";
export default class Test extends LightningElement {
@api part;
}
`;
expect(compile(src)).toThrow(
`LWC1111: Invalid property name "part". "part" is a future reserved attribute for web components.`
);
});

test('both getter and a setter', () => {
const src = /* js */ `
import { api, LightningElement } from "lwc";
export default class Test extends LightningElement {
@api get something() {
return this.s;
}
@api set something(value) {
this.s = value;
}
}
`;
expect(compile(src)).toThrow(
`LWC1112: @api get something and @api set something detected in class declaration. Only one of the two needs to be decorated with @api.`
);
});
});
15 changes: 13 additions & 2 deletions packages/@lwc/ssr-compiler/src/compile-js/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,20 @@ type ExtractArguments<
: ExtractArguments<R, N | Numbers, [string, ...Args]> // `N` already accounted for
: Args; // No `N` found, nothing more to check

class CompilationError extends Error {
constructor(
message: string,
public code: number
) {
super(message);
this.name = 'CompilationError';
this.code = code;
}
}

export function generateError<const T extends LWCErrorInfo>(
error: T,
...args: ExtractArguments<T['message']>
): Error {
return new Error(generateErrorMessage(error, args));
): CompilationError {
return new CompilationError(generateErrorMessage(error, args), error.code);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should use CompilerError from @lwc/errors instead.

}
Original file line number Diff line number Diff line change
Expand Up @@ -145,8 +145,8 @@ export function addGenerateMarkupFunction(
program.body.push(
...bGenerateMarkup(
defaultTagName,
b.arrayExpression(publicFields.map(b.literal)),
b.arrayExpression(privateFields.map(b.literal)),
b.arrayExpression([...publicFields.keys()].map(b.literal)),
b.arrayExpression([...privateFields].map(b.literal)),
classIdentifier,
connectWireAdapterCode
)
Expand Down
92 changes: 86 additions & 6 deletions packages/@lwc/ssr-compiler/src/compile-js/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,23 @@ import type {
Identifier as EsIdentifier,
Program as EsProgram,
Decorator as EsDecorator,
MethodDefinition as EsMethodDefinition,
PropertyDefinition as EsPropertyDefinition,
} from 'estree';
import type { Visitors, ComponentMetaState } from './types';
import type { CompilationMode } from '@lwc/shared';

const DISALLOWED_PROP_SET = new Set(['is', 'class', 'slot', 'style']);

const AMBIGUOUS_PROP_SET = new Map([
['bgcolor', 'bgColor'],
['accesskey', 'accessKey'],
['contenteditable', 'contentEditable'],
['tabindex', 'tabIndex'],
['maxlength', 'maxLength'],
['maxvalue', 'maxValue'],
]);

const visitors: Visitors = {
$: { scope: true },
ExportNamedDeclaration(path) {
Expand Down Expand Up @@ -101,16 +114,21 @@ const visitors: Visitors = {
validateUniqueDecorator(decorators);
const decoratedExpression = decorators?.[0]?.expression;
if (is.identifier(decoratedExpression) && decoratedExpression.name === 'api') {
state.publicFields.push(node.key.name);
if (state.publicFields.has(node.key.name)) {
throw generateError(DecoratorErrors.DUPLICATE_API_PROPERTY, node.key.name);
}
validatePropertyName(node);
validatePropertyValue(node);
state.publicFields.set(node.key.name, node);
} else if (
is.callExpression(decoratedExpression) &&
is.identifier(decoratedExpression.callee) &&
decoratedExpression.callee.name === 'wire'
) {
catalogWireAdapters(path, state);
state.privateFields.push(node.key.name);
state.privateFields.add(node.key.name);
} else {
state.privateFields.push(node.key.name);
state.privateFields.add(node.key.name);
}

if (
Expand Down Expand Up @@ -166,6 +184,28 @@ const visitors: Visitors = {
}
}

if (is.identifier(decoratedExpression, { name: 'api' })) {
const field = state.publicFields.get(node.key.name);

if (field) {
if (
(is.methodDefinition(field, { kind: (k) => k === 'get' || k === 'set' }) &&
node.kind === 'get') ||
node.kind === 'set'
) {
throw generateError(
DecoratorErrors.SINGLE_DECORATOR_ON_SETTER_GETTER_PAIR,
node.key.name
);
} else {
throw generateError(DecoratorErrors.DUPLICATE_API_PROPERTY, node.key.name);
}
}
Copy link
Contributor Author

@cardoso cardoso Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean this up & move to its own function


validatePropertyName(node);
state.publicFields.set(node.key.name, node);
}

switch (node.key.name) {
case 'constructor':
node.value.params = [b.identifier('propsAvailableAtConstruction')];
Expand Down Expand Up @@ -229,9 +269,13 @@ function validateUniqueDecorator(decorators: EsDecorator[]) {

const hasTrack = expressions.some((expr) => is.identifier(expr, { name: 'track' }));

if ((hasWire || hasApi) && hasTrack) {
if (hasWire && hasTrack) {
throw generateError(DecoratorErrors.CONFLICT_WITH_ANOTHER_DECORATOR, 'track');
}

if (hasApi && hasTrack) {
throw generateError(DecoratorErrors.API_AND_TRACK_DECORATOR_CONFLICT);
}
}

export default function compileJS(
Expand All @@ -258,8 +302,8 @@ export default function compileJS(
tmplExplicitImports: null,
cssExplicitImports: null,
staticStylesheetIds: null,
publicFields: [],
privateFields: [],
publicFields: new Map(),
privateFields: new Set(),
wireAdapters: [],
experimentalDynamicComponent: options.experimentalDynamicComponent,
importManager: new ImportManager(),
Expand All @@ -286,3 +330,39 @@ export default function compileJS(
code: generate(ast, {}),
};
}

function isBooleanPropDefaultTrue(property: EsPropertyDefinition) {
const propertyValue = property.value;
return propertyValue && propertyValue.type === 'Literal' && propertyValue.value === true;
}

function validatePropertyValue(property: EsPropertyDefinition) {
if (isBooleanPropDefaultTrue(property)) {
throw generateError(DecoratorErrors.INVALID_BOOLEAN_PUBLIC_PROPERTY);
}
}

function validatePropertyName(property: EsMethodDefinition | EsPropertyDefinition) {
if (property.computed || !('name' in property.key)) {
throw generateError(DecoratorErrors.PROPERTY_CANNOT_BE_COMPUTED);
}

const propertyName = property.key.name;

switch (true) {
case propertyName === 'part':
throw generateError(DecoratorErrors.PROPERTY_NAME_PART_IS_RESERVED, propertyName);
case propertyName.startsWith('on'):
throw generateError(DecoratorErrors.PROPERTY_NAME_CANNOT_START_WITH_ON, propertyName);
case propertyName.startsWith('data') && propertyName.length > 4:
throw generateError(DecoratorErrors.PROPERTY_NAME_CANNOT_START_WITH_DATA, propertyName);
case DISALLOWED_PROP_SET.has(propertyName):
throw generateError(DecoratorErrors.PROPERTY_NAME_IS_RESERVED, propertyName);
case AMBIGUOUS_PROP_SET.has(propertyName):
throw generateError(
DecoratorErrors.PROPERTY_NAME_IS_AMBIGUOUS,
propertyName,
AMBIGUOUS_PROP_SET.get(propertyName)!
);
}
}
cardoso marked this conversation as resolved.
Show resolved Hide resolved
4 changes: 2 additions & 2 deletions packages/@lwc/ssr-compiler/src/compile-js/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,9 @@ export interface ComponentMetaState {
// the set of variable names associated with explicitly imported CSS files
staticStylesheetIds: Set<string> | null;
// the public (`@api`-annotated) fields of the component class
publicFields: Array<string>;
publicFields: Map<string, MethodDefinition | PropertyDefinition>;
// the private fields of the component class
privateFields: Array<string>;
privateFields: Set<string>;
// indicates whether the LightningElement has any wired props
wireAdapters: WireAdapter[];
// dynamic imports configuration
Expand Down