-
Notifications
You must be signed in to change notification settings - Fork 399
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
cardoso
wants to merge
18
commits into
salesforce:master
Choose a base branch
from
cardoso:ssr-validate-api-decorator
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from 17 commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
a6bf833
feat(ssr): validate api decorator
cardoso 5419231
chore: simplify test
cardoso 5049a6c
feat: validate all api decorator errors
cardoso 0409713
chore: move api-decorator tests to its own file
cardoso 84047ff
chore: format test
cardoso 839d89b
Merge branch 'master' of https://github.com/salesforce/lwc into ssr-v…
cardoso aaa8a2e
chore: separate api decorator validation into its own file
cardoso 984f5ed
chore: improve validation functions
cardoso 20c222e
chore: encapsulate api validators
cardoso e5af37c
chore: create api folder
cardoso 2505c01
chore: move decorators to their own folder
cardoso 147e6b0
Merge branch 'master' of https://github.com/salesforce/lwc into ssr-v…
cardoso 9ec59fb
chore: move duplicate consts to @lwc/shared
cardoso d8ade69
Merge branch 'master' of https://github.com/salesforce/lwc into ssr-v…
cardoso c0e8fcd
Merge branch 'master' of https://github.com/salesforce/lwc into ssr-v…
cardoso cbcb9f8
Merge branch 'master' of https://github.com/salesforce/lwc into ssr-v…
cardoso e005a2e
Merge branch 'master' of https://github.com/salesforce/lwc into ssr-v…
cardoso 650562a
Merge branch 'master' of https://github.com/salesforce/lwc into ssr-v…
cardoso File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
168 changes: 168 additions & 0 deletions
168
packages/@lwc/ssr-compiler/src/__tests__/api-decorator.spec.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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"; | ||
}`, | ||
], | ||
])(`%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: 15 additions & 0 deletions
15
packages/@lwc/ssr-compiler/src/compile-js/decorators/api/index.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
/* | ||
* Copyright (c) 2024, Salesforce, Inc. | ||
* All rights reserved. | ||
* SPDX-License-Identifier: MIT | ||
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT | ||
*/ | ||
import type { Decorator, Identifier } from 'estree'; | ||
|
||
export function isApiDecorator(decorator: Decorator | undefined): decorator is Decorator & { | ||
expression: Identifier & { | ||
name: 'api'; | ||
}; | ||
} { | ||
return decorator?.expression.type === 'Identifier' && decorator.expression.name === 'api'; | ||
} |
87 changes: 87 additions & 0 deletions
87
packages/@lwc/ssr-compiler/src/compile-js/decorators/api/validate.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,87 @@ | ||||||||||||||||||||||
/* | ||||||||||||||||||||||
* Copyright (c) 2024, Salesforce, Inc. | ||||||||||||||||||||||
* All rights reserved. | ||||||||||||||||||||||
* SPDX-License-Identifier: MIT | ||||||||||||||||||||||
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT | ||||||||||||||||||||||
*/ | ||||||||||||||||||||||
|
||||||||||||||||||||||
import { DecoratorErrors } from '@lwc/errors'; | ||||||||||||||||||||||
import { DISALLOWED_PROP_SET, AMBIGUOUS_PROP_SET } from '@lwc/shared'; | ||||||||||||||||||||||
import { generateError } from '../../errors'; | ||||||||||||||||||||||
import { type ComponentMetaState } from '../../types'; | ||||||||||||||||||||||
import type { Identifier, MethodDefinition, PropertyDefinition } from 'estree'; | ||||||||||||||||||||||
|
||||||||||||||||||||||
export type ApiMethodDefinition = MethodDefinition & { | ||||||||||||||||||||||
key: Identifier; | ||||||||||||||||||||||
}; | ||||||||||||||||||||||
export type ApiPropertyDefinition = PropertyDefinition & { | ||||||||||||||||||||||
key: Identifier; | ||||||||||||||||||||||
}; | ||||||||||||||||||||||
|
||||||||||||||||||||||
export type ApiDefinition = ApiPropertyDefinition | ApiMethodDefinition; | ||||||||||||||||||||||
|
||||||||||||||||||||||
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)! | ||||||||||||||||||||||
); | ||||||||||||||||||||||
} | ||||||||||||||||||||||
} | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This whole file mirrors the structure and logic from
|
||||||||||||||||||||||
|
||||||||||||||||||||||
function validatePropertyValue(property: ApiPropertyDefinition) { | ||||||||||||||||||||||
if (property.value && property.value.type === 'Literal' && property.value.value === true) { | ||||||||||||||||||||||
throw generateError(DecoratorErrors.INVALID_BOOLEAN_PUBLIC_PROPERTY); | ||||||||||||||||||||||
} | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
function validatePropertyUnique(node: ApiPropertyDefinition, state: ComponentMetaState) { | ||||||||||||||||||||||
if (state.publicFields.has(node.key.name)) { | ||||||||||||||||||||||
throw generateError(DecoratorErrors.DUPLICATE_API_PROPERTY, node.key.name); | ||||||||||||||||||||||
} | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
export function validateApiProperty(node: ApiPropertyDefinition, state: ComponentMetaState) { | ||||||||||||||||||||||
validatePropertyUnique(node, state); | ||||||||||||||||||||||
validateName(node); | ||||||||||||||||||||||
validatePropertyValue(node); | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
function validateUniqueMethod(node: ApiMethodDefinition, state: ComponentMetaState) { | ||||||||||||||||||||||
const field = state.publicFields.get(node.key.name); | ||||||||||||||||||||||
|
||||||||||||||||||||||
if (!field) { | ||||||||||||||||||||||
return; | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
if ( | ||||||||||||||||||||||
field.type === 'MethodDefinition' && | ||||||||||||||||||||||
(field.kind === 'get' || field.kind === 'set') && | ||||||||||||||||||||||
(node.kind === 'get' || node.kind === 'set') | ||||||||||||||||||||||
) { | ||||||||||||||||||||||
throw generateError(DecoratorErrors.SINGLE_DECORATOR_ON_SETTER_GETTER_PAIR, node.key.name); | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
throw generateError(DecoratorErrors.DUPLICATE_API_PROPERTY, node.key.name); | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
export function validateApiMethod(node: ApiMethodDefinition, state: ComponentMetaState) { | ||||||||||||||||||||||
validateUniqueMethod(node, state); | ||||||||||||||||||||||
validateName(node); | ||||||||||||||||||||||
} |
36 changes: 36 additions & 0 deletions
36
packages/@lwc/ssr-compiler/src/compile-js/decorators/index.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
/* | ||
* Copyright (c) 2024, Salesforce, Inc. | ||
* All rights reserved. | ||
* SPDX-License-Identifier: MIT | ||
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT | ||
*/ | ||
import { DecoratorErrors } from '@lwc/errors'; | ||
import { generateError } from '../errors'; | ||
import { isApiDecorator } from './api'; | ||
import { isTrackDecorator } from './track'; | ||
import { isWireDecorator } from './wire'; | ||
import type { Decorator as EsDecorator } from 'estree'; | ||
|
||
export function validateUniqueDecorator(decorators: EsDecorator[]) { | ||
if (decorators.length < 2) { | ||
return; | ||
} | ||
|
||
const hasWire = decorators.some(isWireDecorator); | ||
const hasApi = decorators.some(isApiDecorator); | ||
const hasTrack = decorators.some(isTrackDecorator); | ||
|
||
if (hasWire) { | ||
if (hasApi) { | ||
throw generateError(DecoratorErrors.CONFLICT_WITH_ANOTHER_DECORATOR, 'api'); | ||
} | ||
|
||
if (hasTrack) { | ||
throw generateError(DecoratorErrors.CONFLICT_WITH_ANOTHER_DECORATOR, 'track'); | ||
} | ||
} | ||
|
||
if (hasApi && hasTrack) { | ||
throw generateError(DecoratorErrors.API_AND_TRACK_DECORATOR_CONFLICT); | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe
@lwc/errors
would be a better place for this. Not sure.