Skip to content

Conversation

@abalmush
Copy link

Add the TypeScript override keyword to all Lit component methods that override parent class methods. This is required for Google's internal repository compilation.

Methods updated:

  • render()
  • connectedCallback()
  • disconnectedCallback()
  • firstUpdated()
  • willUpdate()

Fixes #463

Add the TypeScript `override` keyword to all Lit component methods
that override parent class methods. This is required for Google's
internal repository compilation.

Methods updated:
- render()
- connectedCallback()
- disconnectedCallback()
- firstUpdated()
- willUpdate()

Fixes google#463
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request correctly adds the override keyword to Lit component lifecycle methods across multiple files, which is a great improvement for code clarity and to satisfy compiler requirements. My review focuses on a related improvement: adding explicit return types to these same lifecycle methods. Many methods like render, firstUpdated, and willUpdate are missing explicit return types, and adding them would further enhance type safety and maintainability. I've added several suggestions for render and firstUpdated methods. The same principle applies to the multi-line willUpdate signatures, which should also be updated to include a : void return type.

}

render() {
override render() {
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

To improve type safety and code clarity, it's best practice to explicitly define the return type for component lifecycle methods. The render method returns an HTMLTemplateResult, so that should be its explicit return type. You may need to add HTMLTemplateResult to the imports from lit.

Suggested change
override render() {
override render(): HTMLTemplateResult {

}

render() {
override render() {
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

To improve type safety and code clarity, it's best practice to explicitly define the return type for component lifecycle methods. The render method returns an HTMLTemplateResult, so that should be its explicit return type. You may need to add HTMLTemplateResult to the imports from lit.

Suggested change
override render() {
override render(): HTMLTemplateResult {

}

firstUpdated() {
override firstUpdated() {
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

To improve type safety and code clarity, it's best practice to explicitly define the return type for component lifecycle methods. The firstUpdated method should have a void return type.

Suggested change
override firstUpdated() {
override firstUpdated(): void {

}

render() {
override render() {
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

To improve type safety and code clarity, it's best practice to explicitly define the return type for component lifecycle methods. The render method returns an HTMLTemplateResult, so that should be its explicit return type. You may need to add HTMLTemplateResult to the imports from lit.

Suggested change
override render() {
override render(): HTMLTemplateResult {

}

firstUpdated() {
override firstUpdated() {
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

To improve type safety and code clarity, it's best practice to explicitly define the return type for component lifecycle methods. The firstUpdated method should have a void return type.

Suggested change
override firstUpdated() {
override firstUpdated(): void {

}

render() {
override render() {
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

To improve type safety and code clarity, it's best practice to explicitly define the return type for component lifecycle methods. The render method returns an HTMLTemplateResult, so that should be its explicit return type. You may need to add HTMLTemplateResult to the imports from lit.

Suggested change
override render() {
override render(): HTMLTemplateResult {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

All lit components should have override prefixing method/property names if the method/property are inherited from parent

1 participant