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

refactor(decorators): update decorator types @W-16373548@ #4429

Merged
merged 46 commits into from
Sep 6, 2024

Conversation

wjhsf
Copy link
Contributor

@wjhsf wjhsf commented Jul 31, 2024

Details

This PR updates the types for @api, @track, and @wire decorators to only work on components that extend LightningElement. This is technically a breaking change, but should be of minimal impact because the decorators do not work on non-LWC components. Decided to not move forward with this change. Many users extend classes that are typed as any, which is valid but does not satisfy the LightningElement constraint.

This PR also drops support for TypeScript's experimental decorators, requiring projects to use the modern, standard decorators (this is TypeScript's default since v5). This is also technically a breaking change, but because the LWC decorators are compiled into non-standard non-decorator code, the only requirement for pure LWC projects should be removing "experimentalDecorators": true from their tsconfig.json compiler options. This will be an impactful breaking change for LWC projects that use non-LWC experimental decorators on non-LWC classes. I don't know how many users that may be, but it feels like it's probably a small number?

Additionally, this PR makes changes to the @wire decorator to improve type validation. The wire function now validates that the config provided matches the config expected by the adapter, and it validates that the type of the decorated property matches the value emitted by the adapter. In the current version of LWC (v7.2.0), the example below passes type validation. But with the changes in this PR, the latter two uses of @wire would result in type errors.

type Config = { id: number }
type Book = { title: string, author: string }
declare const getBook: WireAdapterConstructor<Config, Book>

class Component extends LightningElement {
  @wire(getBook, { id: 123 }) valid?: Book
  @wire(getBook, { id: 123 }) invalidPropType?: Author
  @wire(getBook, { id: true }) invalidConfigType: Book
}

For bonus fun, reactive config props are accurately resolved! In the following example, '$bookId' is correctly resolved to number, and would pass validation, while '$authorName' is resolved to string, and would fail.

class Component extends LightningElement {
  bookId = 123
  authorName = 'Nolan Lawson'
  @wire(getBook, {id: '$bookId'} as const) valid?: Book
  @wire(getBook, {id: '$authorName'} as const) invalid?: Book
}

Not immediately relevant to this PR, but worth calling out: TypeScript determines the type of the decorator from the type of the property, not the other way around. This means that for @wire(getBook, config) book, the type of book cannot be inferred from getBook; book must always be explicitly typed.

Does this pull request introduce a breaking change?

  • 💔 Yes, it does introduce a breaking change. But only to the types!

Does this pull request introduce an observable change?

  • 🔬 Yes, it does include an observable change.

GUS work item

W-16373548

wjhsf added 25 commits July 24, 2024 11:48
also, only allow @api on LightningElement classes
also, only allow on LightningElements
also, only allow on LightningElements
@wjhsf
Copy link
Contributor Author

wjhsf commented Aug 1, 2024

/nucleus start

@nolanlawson nolanlawson added this to the 8.0.0 milestone Aug 2, 2024
@wjhsf
Copy link
Contributor Author

wjhsf commented Aug 7, 2024

/nucleus start

@wjhsf
Copy link
Contributor Author

wjhsf commented Aug 7, 2024

/nucleus start

@wjhsf
Copy link
Contributor Author

wjhsf commented Aug 7, 2024

/nucleus test

@nolanlawson
Copy link
Collaborator

/nucleus start

Copy link
Collaborator

@nolanlawson nolanlawson left a comment

Choose a reason for hiding this comment

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

This LGTM, but let's hold off on merging it until we've done the Nucleus/core analysis to know the scope of breakage, and are ready to release LWC v8. Awesome work.

// Helper props
configProp = 'config' as const;
nested = { prop: 'config', invalid: 123 } as const;
'nested.prop' = false; // should be unused
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is kind of ambiguous because it's not clear whether $nested.prop refers to line 130 or 131. Better to rename this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is deliberate, to validate that $nested.prop picks up the object, rather than the weird prop name. I'll update the comment to clarify

@nwcm
Copy link

nwcm commented Sep 1, 2024

These are nice improvements. These look specific to the wire, or does this also include better typing for method() calls imperatively?

@wjhsf
Copy link
Contributor Author

wjhsf commented Sep 3, 2024

These are nice improvements. These look specific to the wire, or does this also include better typing for method() calls imperatively?

These changes are specific to @wire. Can you elaborate on what you mean by method() calls imperatively?

@wjhsf wjhsf merged commit d79c246 into master Sep 6, 2024
11 checks passed
@wjhsf wjhsf deleted the wjh/modern-decorators branch September 6, 2024 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants