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

Implicit override declarations #2026

Merged
merged 18 commits into from
Jun 24, 2024
Merged

Conversation

nielslyngsoe
Copy link
Member

Enforce declaring when overriding a method or property.

Description

This makes sure an override is done deliberately.
Doing this already spotted quite a few places where things where double declared, which would be troublesome maintaining over time.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (minor updates related to the tooling or maintenance of the repository, does not impact compiled assets)

Motivation and context

Ensuring awareness on what is begin extended and ensuring the maintainability of such.

How to test?

Screenshots (if appropriate)

Checklist

  • If my change requires a change to the documentation, I have updated the documentation in this pull request.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.

Copy link
Collaborator

@iOvergaard iOvergaard left a comment

Choose a reason for hiding this comment

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

Not sure about the immediate need for all of this. "Styles" is a requirement to define when you extend a component, so that's good, but I'm sure there could be use-cases where you don't want to override render() or event connectedCAllback(). I'm afraid this causes a lot of overhead for ourselves without bringing a whole lot of value to others. How do you see this?

If we are to change anything about styles I would also recommend to add the readonly keyword to the mix to prevent people from overwriting the static variable from the outside.

@nielslyngsoe
Copy link
Member Author

nielslyngsoe commented Jun 21, 2024

@iOvergaard good idea adding readonly for the styles, I could make that in another PR — I'm thought not sure we can enforce it to be so, but it would be good for our components to do so. (Update, in relation to the idea below, then there should be an option for us to make a type overwrite of our lit element, to re-declare styles so it becomes readonly in TypeScript)

The need for the explicitly declaring override, is to ensure knowledge.
This helps the developer begin aware about these things:

  • If a method or property of the given name is already declared via what is inherited.
    This as well makes the developer aware, and hopefully they will consider that they might need to call super.method()
  • If a we append a new method or property in a existing class, then we get to know if the name already was used by a more specific implementation.
  • A few double declarations has already been capture by configuring this, double declarations that would be have been very hard to spot otherwise. One example is a state that was declared in a base class and again in a specific class leading to some cases that is very hard to debug, that could be observables that is based on another state than the one mainly used in the implementation.

It is one more word to write, but I made add that C# developers are used to this. And I, myself, have previously written other language where this also was mandatory.

Long term value is to avoid that our further development gets into trouble when appending or removing things in classes that are begin extended. I think its unrealistic to assume that we get around to ensure that there is no existing overrides(thing with the same name) of the things we append/correct — As well we already had some of these issues, discovered by this PR.

I was at first also skeptical, mainly because its needed for render and styles, which I do not directly see as overwrites — the only reason they are overwrites is because Lit comes with a default implementation, and it is not mandatory to override, aka. they are not abstract. I have investigated a few ways to get around those specifically but I did not find a solution jet. But I could look further into it.
The only solution I could some up with, was to make a LitElement that was abstract and then mark those two as abstracts, this would enforce us to always declare these, but we almost always do that any way.

:-)

@iOvergaard iOvergaard changed the title Implicit overwrite declarations Implicit override declarations Jun 24, 2024
Copy link
Collaborator

@iOvergaard iOvergaard left a comment

Choose a reason for hiding this comment

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

I just read up on it, because I didn't know it was a thing in TypeScript with "noImplicitOverride" and I agree, it makes sense to have, although I am a bit sorry we have to add it for all Lit methods.

If a method or property of the given name is already declared via what is inherited.
This as well makes the developer aware, and hopefully they will consider that they might need to call super.method()
If a we append a new method or property in a existing class, then we get to know if the name already was used by a more specific implementation.

These added values are nice for us, but to my understanding, people have to turn on "noImplicitOverride" in their own tsconfig to get the same result, so just as long as we are aware of that it only helps us to maintain a large codebase, it's fine.

LGTM.

@iOvergaard iOvergaard added the internal Changes that do not affect the runtime files, e.g. changes to the build setup label Jun 24, 2024
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://ambitious-stone-0033b3603-2026.westeurope.1.azurestaticapps.net

@nielslyngsoe nielslyngsoe merged commit c1a11b1 into main Jun 24, 2024
6 checks passed
@nielslyngsoe nielslyngsoe deleted the v14/chore/implicit-overwrite branch June 24, 2024 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal Changes that do not affect the runtime files, e.g. changes to the build setup storybook
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants