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] Reset property to its initial value when attribute is removed. #4643

Open
1 task done
rictic opened this issue May 9, 2024 · 8 comments
Open
1 task done
Labels

Comments

@rictic
Copy link
Collaborator

rictic commented May 9, 2024

Should this be an RFC?

  • This is not a substantial change

Which package is this a feature request for?

Lit Core (lit / lit-html / lit-element / reactive-element)

Description

Consider a simple element:

@customElement('test-comp')
export class TestComp extends LitElement {
  @property() name = 'Alex';

  render() {
    return html`<p>Hello, ${this.name}!</p>`;
  }
}

If a user of the element adds a name attribute, and then removes it, the component ends up with name set to null. This is surprising, is difficult to type check, and different from how native HTML elements typically work.

In a future version of Lit where we can assume standard decorators, we'll have access to the initial value, and we can store that away, so that when the attribute is removed we reset back to the initial value of the property. This is a breaking change, but it moves towards a less surprising result, and most places that are depending on this are likely doing so accidentally.

Alternatives and Workarounds

You would have to write a custom attribute converter.

@rictic rictic changed the title [standard decorators] Reset property to its initial value when attribute is removed. [reactive-element] Reset property to its initial value when attribute is removed. May 9, 2024
@rictic rictic added the 4.0 label May 9, 2024
@augustjk
Copy link
Member

augustjk commented May 9, 2024

Related? lit/rfcs#36

@justinfagnani
Copy link
Collaborator

I'm not sure we need to wait for standard decorators only to do this.

The danger of keeping the initial value around us that it could leak a lot of memory for complex objects, so we want people to opt into this. Once they have to do that, they can provide the default value to the decorator and but use the initializer. That has the benefit of one shared default value for the whole class, not each instance.

We could do that now.

@sorvell
Copy link
Member

sorvell commented May 10, 2024

It's possible to do this with a custom attribute converter, but we can definitely consider supporting this more directly. Here's an example for reference:

playground example that uses:

const {fromAttribute} = defaultConverter;
export const withFallback = (fallback, from = fromAttribute) => ({
  ...defaultConverter,
  fromAttribute: (value: string, type: unknown) => 
    from(value, type) ?? fallback
});

//...

const fooDefault = 'foo';
@customElement('my-element')
class MyElement extends LitElement { 
  @property({converter: withFallback(fooDefault)})
  foo = fooDefault
  //...

@KonnorRogers
Copy link

+1 to this.

we have a massive hole in Shoelace in regards to "morphing" where the server ends up removing attributes from the client, causing us to end up with "null" attributes when we sprout attributes.

Happy to provide references and examples, just don't want to take over someone else's RFC :)

@sorvell
Copy link
Member

sorvell commented May 12, 2024

Description of the morphing problem

@KonnorRogers
Copy link

@sorvell after running the playground it appears the "attribute" ("message" in this case) still gets removed from the host element, but the internal property stays the same. I tried implementing a toAttribute() that used the default toAttribute converter, but with no luck.

Right now the best I could come up with for a synchronous version would be using getters / setter.

static properties = { foo: { reflect: true }}

constructor () {
  super()
  this.__foo__ = "bar"
  this.requestUpdate("foo")
}

get foo () {
  return this.__foo__
}

set foo (val) {
  if (val == null) {
    val = "bar"
    this.setAttribute("foo", val)
  }
  
  this.foo = val
}

^ Super hacky and gross, but technically works the way I want it to.

@sorvell
Copy link
Member

sorvell commented Jun 17, 2024

Want to make sure I understand. Can you describe the exact steps taken and the unexpected result?

@sorvell
Copy link
Member

sorvell commented Jun 17, 2024

Looking at this again, yeah, it definitely gets a bit tricky. In particular, it would be easy to create a loop between from/to attribute so the internal code tries hard not to do that.

Let me see if I understand...

  • With the setter you wrote, the attribute is set to a default value but the property is set to null. Is that important? I'm guessing no?
  • However, in the playground example, if you remove the attribute (what you're concerned with the server doing), it's actually removed, which is a problem.

If so, this morphing case seems different from the issue here, which is about returning to a default property value when the attribute is removed.

In the morphing case, it's not that you want to return to a default property value when the attribute is removed, but instead you want an attribute removal to effectively be invalid? So, whenever the attribute is removed, the property would not change and the attribute would be restored to the property's current value?

If this is correct... I think you might instead benefit from implementing attributeChangedCallback directly like this playground:

attributeChangedCallback(name, old, value) {
  if (value !== null) {
    super.attributeChangedCallback(name, old, value);
    return;
  }
  const {reflect, type} = this.constructor.getPropertyOptions(name) ?? {};
  if (reflect && !(type === Boolean && !this[name])) {
    this.setAttribute(name, old);
  } else {
    super.attributeChangedCallback(name, old, value);
  }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: No status
Development

No branches or pull requests

5 participants