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

Proposal: default/fallback values for properties #905

Closed
javier-garcia-meteologica opened this issue Feb 27, 2020 · 3 comments
Closed

Proposal: default/fallback values for properties #905

javier-garcia-meteologica opened this issue Feb 27, 2020 · 3 comments

Comments

@javier-garcia-meteologica
Copy link

javier-garcia-meteologica commented Feb 27, 2020

Problem

Many of our bugs are related to people believing that the initialization value is a default value.

@component('filter')
export class Filter extends LitElement {
  @property({ type: Number })
  min: number = -Infinity;

  @property({ type: Number })
  max: number = Infinity;

  @property({ type: Number })
  decimals: number = 0;

  ...

  onSelectorChanged (e: CustomEvent) {
     const value = e.detail.value;
     // Everyone expects min and max to be defined numbers, never to be undefined
     // But when it is undefined, all hell breaks loose
     if (value < this.min || value > this.max) {
       ...
     }
     this.value = value;
  }
}

So they write code, that works. But it is very ugly and repetitive.

  get _max () {
    return this.max !== undefined ? this.max : Infinity;
  }
  get _min () {
    return this.min !== undefined ? this.min : -Infinity;
  }

  onSelectorChanged (e: CustomEvent) {
     const value = e.detail.value;
     if (value < this._min || value > this._max) {
       ...
     }
     this.value = value;
  }

Why are so many variables set as undefined despite having an initialization value? Because of abstractions. Let's say we have a filters property with many dynamic filters to be rendered.

get filtersTemplate () {
  return this.filters.map(filter => {
    return html`<filter
      .min="${filter.min}" 
      .max="${filter.max}" 
      .decimals="${filter.decimals}" >
    </filter>`
  });
}

Any filter property that is undefined will override the initialization value of our filter component. The following syntax would be great, but it's not legal.

// This wil throw
html`<filter ${ filter.min !== undefined ? 'min="${filter.min}"' : ''}"></filter>`

Proposal

We use a patched version of lit-element that allows the following syntax.

@component('filter')
export class Filter extends LitElement {
  @property({ type: Number, default: () => -Infinity })
  min: number;

  @property({ type: Number, default: () => Infinity })
  max: number;

  @property({ type: Number, default: () => ({ foo: 'bar' }) })
  extraData: number;

  ...

  onSelectorChanged (e: CustomEvent) {
     const value = e.detail.value;
    // Works as expected
     if (value < this.min || value > this.max) {
       ...
     }
     this.value = value;
  }
}

Whenever a property would become undefined, the value is overridden with the default value.

Take a look at branch default_values in my repo.

I've also opened this pull request to discuss implementation details #906

@Westbrook
Copy link
Contributor

Isn’t this what the ifDefined decorator is for?

 min="${ifDefined(filter.min)}" 

The above usage only sets the min attribute when the value of filter.min is defined.

@javier-garcia-meteologica
Copy link
Author

javier-garcia-meteologica commented Feb 27, 2020

Thank you @Westbrook, I personally didn't know about the lit-html directive isDefined. That directive soothes the underlying problem. However I think this proposal can still be a useful addition to lit-element.

Everything is in the same component/file with the default option, so other people know what to expect.

Apart from the scenario described above, this default directive applies to everything. There may be cases in which you want to reset the property of a component from a parent, and you don't know which is the default value of the child property. Or you just want to reduce verbosity.

onSelectorChanged (e: CustomEvent) {
  // No need to check if value is undefined, if so it will write the default value
  this.value = e.detail.value;
}

All in all, I think this solution can help programmers write simpler code.

@sorvell
Copy link
Member

sorvell commented Mar 3, 2020

Thanks for the suggestion. We'd like to keep the core library as small as possible so we're not inclined to add this feature right now. However, we're definitely interested in making this kind of customization easier. We're planning some improvements on that front in #911 so we'll close this issue in favor of that one.

@sorvell sorvell closed this as completed Mar 3, 2020
sorvell pushed a commit that referenced this issue Mar 5, 2020
Fixes #905. A couple changes to make customizing property accessors easier.

* Adds static methods`setOptionsForProperty(name, options)`, `getOptionsForProperty` and renames `_requestUpdate` to `requestUpdateInternal` and makes it protected.

With these changes, users can override `createProperty` and (1) pass in and later retrieve custom property otions, (2) customize how accessors are defined and call `requestUpdateInternal` in the setter to make the property reactive.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants