feat(ssr-compiler): validate api decorator usage by cardoso · Pull Request #5071 · salesforce/lwc

cardoso · 2024-12-20T17:08:42Z

Details

I'm a bit short on time today to finish polishing this up, so I'm sending this as a draft.

Follow up to #5062 and #5114
Partially addresses #5032

This PR aims to replicate the api decorator validation done in babel-plugin-component and also related tests.

Some of the validation done in babel-plugin-component seems to be done after everything is collected, while here it's currently all done during traversal. Maybe that's just for better error reporting, which I think can be considered in a separate issue, but need to make sure this is not introducing undesirable behavior here.

Does this pull request introduce a breaking change?

😮‍💨 No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

🤞 No, it does not introduce an observable change.

GUS work item

packages/@lwc/ssr-compiler/src/compile-js/index.ts

…alidate-api-decorator

cardoso · 2025-01-07T16:35:52Z

I'm a bit unsure about the scope of this PR, so I'm opening it up for review.

The main remaining TODO is to replicate the valid tests, but on second thought that might already be covered by the engine-server test suite?

cardoso

The approach taken here is to mirror behavior and tests from babel-plugin-component. I focused on the api decorator errors and didn't introduce location info to keep the PR short.

cardoso · 2025-01-07T16:40:20Z

packages/@lwc/shared/src/html-attributes.ts

+
+/**
+ * This set is for attributes that have a camel cased property name
+ * For example, div.tabIndex.
+ * We do not want users to define `@api` properties with these names
+ * Because the template will never call them. It'll always call the camel
+ * cased version.
+ */
+export const AMBIGUOUS_PROP_SET = /*@__PURE__@*/ new Map([
+    ['bgcolor', 'bgColor'],
+    ['accesskey', 'accessKey'],
+    ['contenteditable', 'contentEditable'],
+    ['tabindex', 'tabIndex'],
+    ['maxlength', 'maxLength'],
+    ['maxvalue', 'maxValue'],
+]);
+
+/**
+ * This set is for attributes that can never be defined
+ * by users on their components.
+ * We throw for these.
+ */
+export const DISALLOWED_PROP_SET = /*@__PURE__@*/ new Set(['is', 'class', 'slot', 'style']);


Maybe @lwc/errors would be a better place for this. Not sure.

This isn't specifically related to errors, @lwc/shared is fine.

packages/@lwc/ssr-compiler/src/__tests__/api-decorator.spec.ts

cardoso · 2025-01-07T16:50:10Z

packages/@lwc/ssr-compiler/src/compile-js/decorators/api/validate.ts

+function validateName(definition: ApiDefinition) {
+    if (definition.computed) {
+        throw generateError(DecoratorErrors.PROPERTY_CANNOT_BE_COMPUTED);
+    }
+
+    const propertyName = definition.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)!
+            );
+    }
+}


This whole file mirrors the structure and logic from babel-plugin-component:

lwc/packages/@lwc/babel-plugin-component/src/decorators/api/validate.ts

Lines 61 to 70 in b2468c9

function validatePropertyName(property: NodePath<types.ClassMethod>, state: LwcBabelPluginPass) {

if (property.node.computed) {

throw generateError(

property,

{

errorInfo: DecoratorErrors.PROPERTY_CANNOT_BE_COMPUTED,

},

state

);

}

…alidate-api-decorator

cardoso

Moved back to draft as I learned some stuff in #5114 that will make this better 😅

packages/@lwc/ssr-compiler/src/compile-js/errors.ts

…alidate-api-decorator

wjhsf

I'll need to have a second look, later, to fully parse this, but here's a few minor code quality comments from a quick glance at this.

wjhsf · 2025-01-28T19:24:30Z

packages/@lwc/shared/src/html-attributes.ts

+
+/**
+ * This set is for attributes that have a camel cased property name
+ * For example, div.tabIndex.
+ * We do not want users to define `@api` properties with these names
+ * Because the template will never call them. It'll always call the camel
+ * cased version.
+ */
+export const AMBIGUOUS_PROP_SET = /*@__PURE__@*/ new Map([
+    ['bgcolor', 'bgColor'],
+    ['accesskey', 'accessKey'],
+    ['contenteditable', 'contentEditable'],
+    ['tabindex', 'tabIndex'],
+    ['maxlength', 'maxLength'],
+    ['maxvalue', 'maxValue'],
+]);
+
+/**
+ * This set is for attributes that can never be defined
+ * by users on their components.
+ * We throw for these.
+ */
+export const DISALLOWED_PROP_SET = /*@__PURE__@*/ new Set(['is', 'class', 'slot', 'style']);


This isn't specifically related to errors, @lwc/shared is fine.

packages/@lwc/ssr-compiler/src/compile-js/decorators/index.ts

wjhsf · 2025-01-28T19:30:34Z

packages/@lwc/ssr-compiler/src/compile-js/decorators/api/validate.ts

+
+    const propertyName = definition.key.name;
+
+    switch (true) {


Why switch (true) rather than a regular if/else?

It's just more readable and maintainable. Here's the code it's based on in babel-plugin-component:

lwc/packages/@lwc/babel-plugin-component/src/decorators/api/validate.ts

Lines 70 to 117 in 9e8bcc0

if (propertyName === 'part') {

throw generateError(

property,

{

errorInfo: DecoratorErrors.PROPERTY_NAME_PART_IS_RESERVED,

messageArgs: [propertyName],

},

state

);

} else if (propertyName.startsWith('on')) {

throw generateError(

property,

{

errorInfo: DecoratorErrors.PROPERTY_NAME_CANNOT_START_WITH_ON,

messageArgs: [propertyName],

},

state

);

} else if (propertyName.startsWith('data') && propertyName.length > 4) {

throw generateError(

property,

{

errorInfo: DecoratorErrors.PROPERTY_NAME_CANNOT_START_WITH_DATA,

messageArgs: [propertyName],

},

state

);

} else if (DISALLOWED_PROP_SET.has(propertyName)) {

throw generateError(

property,

{

errorInfo: DecoratorErrors.PROPERTY_NAME_IS_RESERVED,

messageArgs: [propertyName],

},

state

);

} else if (AMBIGUOUS_PROP_SET.has(propertyName)) {

const camelCased = AMBIGUOUS_PROP_SET.get(propertyName);

throw generateError(

property,

{

errorInfo: DecoratorErrors.PROPERTY_NAME_IS_AMBIGUOUS,

messageArgs: [propertyName, camelCased],

},

state

);

}

}

I've used this pattern on occasion and I'm generally okay with it. However, if other folks on the team prefer the if/else pattern, that's fine with me too. Sometimes it's better to conform to the norm of the codebase, even if something else would be more easily readable.

Yeah, I don't feel strongly about this. It was just easier for me to write and understand what was going on in the original code.

packages/@lwc/ssr-compiler/src/compile-js/index.ts

packages/@lwc/ssr-compiler/src/compile-js/types.ts

packages/@lwc/ssr-compiler/src/compile-js/decorators/api/validate.ts

…alidate-api-decorator

Co-authored-by: Will Harney <62956339+wjhsf@users.noreply.github.com>

divmain

No objections to this PR, but I'll let Will give the final 👍 once he is satisfied.

divmain · 2025-01-30T01:00:29Z

packages/@lwc/ssr-compiler/src/compile-js/decorators/api/validate.ts

+
+    const propertyName = definition.key.name;
+
+    switch (true) {


I've used this pattern on occasion and I'm generally okay with it. However, if other folks on the team prefer the if/else pattern, that's fine with me too. Sometimes it's better to conform to the norm of the codebase, even if something else would be more easily readable.

wjhsf · 2025-01-31T16:30:24Z

/nucleus test

…alidate-api-decorator

wjhsf · 2025-02-06T22:01:12Z

/nucleus test

cardoso added 5 commits December 19, 2024 20:09

feat(ssr): validate api decorator

a6bf833

chore: simplify test

5419231

feat: validate all api decorator errors

5049a6c

chore: move api-decorator tests to its own file

0409713

chore: format test

84047ff

cardoso requested a review from a team as a code owner December 20, 2024 17:08

salesforce-cla bot added the cla:signed label Dec 20, 2024

cardoso marked this pull request as draft December 20, 2024 17:08

cardoso commented Dec 20, 2024

View reviewed changes

packages/@lwc/ssr-compiler/src/compile-js/index.ts Outdated Show resolved Hide resolved

cardoso commented Dec 20, 2024

View reviewed changes

packages/@lwc/ssr-compiler/src/compile-js/index.ts Show resolved Hide resolved

cardoso added 10 commits December 20, 2024 16:13

Merge branch 'master' of https://github.com/salesforce/lwc into ssr-v…

839d89b

…alidate-api-decorator

chore: separate api decorator validation into its own file

aaa8a2e

chore: improve validation functions

984f5ed

chore: encapsulate api validators

20c222e

chore: create api folder

e5af37c

chore: move decorators to their own folder

2505c01

Merge branch 'master' of https://github.com/salesforce/lwc into ssr-v…

147e6b0

…alidate-api-decorator

chore: move duplicate consts to @lwc/shared

9ec59fb

Merge branch 'master' of https://github.com/salesforce/lwc into ssr-v…

d8ade69

…alidate-api-decorator

Merge branch 'master' of https://github.com/salesforce/lwc into ssr-v…

c0e8fcd

…alidate-api-decorator

cardoso marked this pull request as ready for review January 7, 2025 16:32

cardoso commented Jan 7, 2025

View reviewed changes

cardoso added 2 commits January 7, 2025 15:25

Merge branch 'master' of https://github.com/salesforce/lwc into ssr-v…

cbcb9f8

…alidate-api-decorator

Merge branch 'master' of https://github.com/salesforce/lwc into ssr-v…

e005a2e

…alidate-api-decorator

cardoso marked this pull request as draft January 8, 2025 21:09

cardoso commented Jan 8, 2025

View reviewed changes

packages/@lwc/ssr-compiler/src/compile-js/errors.ts Outdated Show resolved Hide resolved

cardoso added 3 commits January 10, 2025 15:58

Merge branch 'master' of https://github.com/salesforce/lwc into ssr-v…

650562a

…alidate-api-decorator

Merge branch 'master' of https://github.com/salesforce/lwc into ssr-v…

16d0237

…alidate-api-decorator

chore: remove commented code

a41a646

cardoso marked this pull request as ready for review January 28, 2025 12:43

wjhsf reviewed Jan 28, 2025

View reviewed changes

cardoso and others added 5 commits January 29, 2025 09:54

Merge branch 'master' of https://github.com/salesforce/lwc into ssr-v…

9e8bcc0

…alidate-api-decorator

chore: simplify publicField types

5026ce3

Co-authored-by: Will Harney <62956339+wjhsf@users.noreply.github.com>

chore: use is.identifier

65b1e0b

Co-authored-by: Will Harney <62956339+wjhsf@users.noreply.github.com>

chore: use is.literal

2448ecf

Co-authored-by: Will Harney <62956339+wjhsf@users.noreply.github.com>

fix: type errors

a65939d

cardoso requested a review from wjhsf January 29, 2025 13:34

divmain reviewed Jan 30, 2025

View reviewed changes

wjhsf mentioned this pull request Jan 31, 2025

[DO NOT MERGE] Trigger CI for #5071: feat(ssr-compiler): validate api decorator usage #5180

Closed

wjhsf approved these changes Jan 31, 2025

View reviewed changes

cardoso added 2 commits January 31, 2025 14:10

Merge branch 'master' of https://github.com/salesforce/lwc into ssr-v…

e7f83cb

…alidate-api-decorator

Merge branch 'master' of https://github.com/salesforce/lwc into ssr-v…

65f8324

…alidate-api-decorator

cardoso requested review from divmain and wjhsf February 6, 2025 21:09

wjhsf approved these changes Feb 6, 2025

View reviewed changes

wjhsf mentioned this pull request Feb 6, 2025

[DO NOT MERGE] Trigger CI for #5071: feat(ssr-compiler): validate api decorator usage #5200

Closed

wjhsf merged commit 62fecca into salesforce:master Feb 7, 2025
11 checks passed

	function validatePropertyName(property: NodePath<types.ClassMethod>, state: LwcBabelPluginPass) {
	if (property.node.computed) {
	throw generateError(
	property,
	{
	errorInfo: DecoratorErrors.PROPERTY_CANNOT_BE_COMPUTED,
	},
	state
	);
	}

	if (propertyName === 'part') {
	throw generateError(
	property,
	{
	errorInfo: DecoratorErrors.PROPERTY_NAME_PART_IS_RESERVED,
	messageArgs: [propertyName],
	},
	state
	);
	} else if (propertyName.startsWith('on')) {
	throw generateError(
	property,
	{
	errorInfo: DecoratorErrors.PROPERTY_NAME_CANNOT_START_WITH_ON,
	messageArgs: [propertyName],
	},
	state
	);
	} else if (propertyName.startsWith('data') && propertyName.length > 4) {
	throw generateError(
	property,
	{
	errorInfo: DecoratorErrors.PROPERTY_NAME_CANNOT_START_WITH_DATA,
	messageArgs: [propertyName],
	},
	state
	);
	} else if (DISALLOWED_PROP_SET.has(propertyName)) {
	throw generateError(
	property,
	{
	errorInfo: DecoratorErrors.PROPERTY_NAME_IS_RESERVED,
	messageArgs: [propertyName],
	},
	state
	);
	} else if (AMBIGUOUS_PROP_SET.has(propertyName)) {
	const camelCased = AMBIGUOUS_PROP_SET.get(propertyName);
	throw generateError(
	property,
	{
	errorInfo: DecoratorErrors.PROPERTY_NAME_IS_AMBIGUOUS,
	messageArgs: [propertyName, camelCased],
	},
	state
	);
	}
	}

Conversation

cardoso commented Dec 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Does this pull request introduce a breaking change?

Does this pull request introduce an observable change?

GUS work item

Uh oh!

Uh oh!

Uh oh!

cardoso commented Jan 7, 2025

Uh oh!

cardoso left a comment

Choose a reason for hiding this comment

Uh oh!

cardoso Jan 7, 2025

Choose a reason for hiding this comment

Uh oh!

wjhsf Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cardoso Jan 7, 2025

Choose a reason for hiding this comment

Uh oh!

cardoso left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wjhsf left a comment

Choose a reason for hiding this comment

Uh oh!

wjhsf Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wjhsf Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

cardoso Jan 29, 2025

Choose a reason for hiding this comment

Uh oh!

divmain Jan 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cardoso Jan 31, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

divmain left a comment

Choose a reason for hiding this comment

Uh oh!

divmain Jan 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wjhsf commented Jan 31, 2025

Uh oh!

wjhsf commented Feb 6, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cardoso commented Dec 20, 2024 •

edited

Loading

divmain Jan 30, 2025 •

edited

Loading

divmain Jan 30, 2025 •

edited

Loading