-
Notifications
You must be signed in to change notification settings - Fork 780
fix: add override keyword to Lit component lifecycle methods #535
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
base: main
Are you sure you want to change the base?
Conversation
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
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.
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() { |
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.
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.
| override render() { | |
| override render(): HTMLTemplateResult { |
| } | ||
|
|
||
| render() { | ||
| override render() { |
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.
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.
| override render() { | |
| override render(): HTMLTemplateResult { |
| } | ||
|
|
||
| firstUpdated() { | ||
| override firstUpdated() { |
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.
| } | ||
|
|
||
| render() { | ||
| override render() { |
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.
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.
| override render() { | |
| override render(): HTMLTemplateResult { |
| } | ||
|
|
||
| firstUpdated() { | ||
| override firstUpdated() { |
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.
| } | ||
|
|
||
| render() { | ||
| override render() { |
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.
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.
| override render() { | |
| override render(): HTMLTemplateResult { |
Add the TypeScript
overridekeyword to all Lit component methods that override parent class methods. This is required for Google's internal repository compilation.Methods updated:
Fixes #463