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

[reactive-element] Resolve issues with setting default values for reflecting properties #1476

Open
sorvell opened this issue Dec 10, 2020 · 8 comments
Assignees

Comments

@sorvell
Copy link
Member

sorvell commented Dec 10, 2020

Description

It's currently tricky to match native DOM behavior when setting a default value for a reflecting property. For example, the input element's type property defaults to text but does not reflect to an attribute by default. If a reflecting property gets a default value in UpdatingElement it will reflect its initial value unlike input's type property.

It's possible to customize this behavior by implementing a getter/setter and/or customizing the generated accessor via getPropertyDescriptor, but it's fairly tricky to achieve the right behavior.

One possibility is that a default property option could be added to the property declaration. This would seed the initial value of the property without calling the setter and seeding the reflection. The downside is that it makes property initialization non-standard (e.g. it's normal to initialize the value via a class field or in the constructor).

Additionally, sometimes users want a default value to be used when the property value is set to undefined or "invalid" value. Opinions differ on this so it might be better to handle separately.

Acceptance criteria

Decide on the right approach and land PR.

@sorvell sorvell self-assigned this Dec 10, 2020
@justinfagnani justinfagnani changed the title [lit-next] Resolve issues with setting default values for reflecting properties [reactive-element] Resolve issues with setting default values for reflecting properties Jan 15, 2021
@yannbf
Copy link

yannbf commented May 17, 2021

Hey peeps! Just out of curiosity, has there been any development related to this issue so far?

@justinfagnani
Copy link
Collaborator

Maybe we can add a wrapper object that tells our setters to not reflect. I think we can get TypeScript to be happy with it. Something like:

class MyElement extends LitElement {
  @property() foo = default(1);
}

@justinfagnani justinfagnani added this to the Lit 2.1 milestone Aug 20, 2021
@justinfagnani
Copy link
Collaborator

Or:

class MyElement extends LitElement {
  @property({defaulted: true}) foo = 1;
}

@KonnorRogers
Copy link

KonnorRogers commented Nov 5, 2022

I'm looking at the ergonomics of properties this issue struck me as the closest thing. What if we had a syntax like this:

class MyElement extends LitElement {
  static properties = {
    theme: { default: "dark" }
  }
}

This has the added benefit of someone could dig into the constructor to get the initial value.

myElement.constructor.properties.theme.default

it also means that we don't need two call sites for non-decorator users.

this also would allow objects to be shared easier since you don't have to both define the property and then initialize a default in the constructor.

As for decorators, I imagine "default" would just get populated with whatever's passed as the initial value.

@property() theme = "dark"

would translate to:

{ default: "dark" }

I guess the only question is what happens when a value is assigned twice?

class MyElement extends LitElement {
  static properties = {
    theme: { default: "dark" }
  }

  constructor () {
    super() 
    // this.theme = "dark" should be set in the super call I imagine?

    this.theme = "light"
  }
}

am I missing something obvious here??

Also, should I make a new issue for this? It feels tangentially related, but not sure if it's for the same reasons.

@WickyNilliams
Copy link
Contributor

WickyNilliams commented Mar 29, 2023

I keep wishing there was a way to solve this. Many of my components spontaneously reflect because there is currently no way to mimic the behaviour of native elements. And the ergnomics of having an explicit initial value in code are too nice to avoid.

Either of the proposed solutions sound good to me. Though I'm wondering if the function approach default(1) might pose a problem to downstream tooling (e.g. would CEM need to be adapted to understand this?)

@justinfagnani
Copy link
Collaborator

We're addressing this in Lit 3.0 with the new standard decorator implementations. Standard decorators get access to the initial value and can skip reflection for those values. This is being done in #3982

@WickyNilliams
Copy link
Contributor

Very cool! Thanks for the update

@justinfagnani
Copy link
Collaborator

Update: we're going to address this in a follow up to 3.0.

We were initially going to solve this with the standard decorators implementations which can differentiate initial values. Instead we decided to keep the behavior of experimental decorators and standard decorators as close as possible to enable switching back and forth once the callsites have been updated to use accessor.

This means we can't rely on the standard decorator for this fix. Also, when I implemented the behavior of not reflecting initial values with standard decorators I got nervous about also using initial values as the "default" value so that removing the associated attribute set the property back to the default - this is because to do that we have to remember the initial value and that could hold onto an unexpected amount of memory in some cases (initializing fields w/o attribute: false to large objects).

I think the fix going forward is to make non-reflecting initial values and default values tied together, opt-in, and work with experimental decorators too. The way to do this is to add a new default property option, that holds a value that's shared across all instances.

@justinfagnani justinfagnani removed the 3.0 label Sep 20, 2023
@justinfagnani justinfagnani removed this from the Lit 2.1 milestone Sep 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 📋 Triaged
Development

No branches or pull requests

6 participants