Decide on how to implement computed properties

[justinfagnani]

We have two PRs that add computed properties in two different ways: #18 and #20.

Both have their merits, and slightly different capabilities, and also we'd like to see if we can get some more type safety from compute function declarations.

@aomarks and I played with a few options yesterday, none of which seemed like a slam dunk. I wanted to open the issue here so we could all discuss the pros and cons, with @43081j and @danielvanmil. @kito99 might be interested too.

There's a few facts that come into play, and are a bit in tension:

• TypeScript needs either a property or getter declaration in order to see a property on a class.
• A computing function needs arguments in order to receive change records from Polymer
• Neither properties nor getters accept arguments, obviously
• Polymer needs the dependency names of the computing function
• It's considered a best practice that the computing function is pure
• It's basically not possible to make TypeScript type-check the computing functions arguments against the properties of the class, or the parameter names passed to Polymer.

@computed()

The getter approach in #20 is pretty nice on the TypeScript side because we don't use arguments that can't be verified by tsc:

class X {
  @property() a: number;
  @property() b: number;
  @computed('a', 'b') get c(): number { return this.a + this.b; }
}

Pros:

• The implementation of c is fully type-checked.
• Simplicity: there's only one declaration and one type of option to the decorator - the dependencies.

Cons:

• We can't verify that it only uses instance fields that it's declared dependencies on, at least without our own linter.
• Being a getter is a little misleading, because it's not called with c is accessed, but when a and b change, as Polymer caches the value.
• The computing function can't accept change records for dependencies like a.*
• We need to add options to @computed that are valid from @property, like reflectToAttribute.

@Property()

The @property() options approach in #18 is more similar to standard Polymer:

class X {
  @property() a: number;
  @property() b: number;
  @property({computed: `_c(a,b)`})
  readonly c: number;
  _c(a: number, b: number): number {
    return a + b;
  }
}

Pros:

• Allows the full power of Polymer computed properties, including wildcards
• Already allows setting reflectToAttribute

Cons:

• More verbose. We need to declare both the property and the computing function, and the @property decoration is longer.
• @property() admits invalid options. type and readonly aren't needed.
• Computed function parameters aren't type checked. We can't tell TypeScript that they correspond to instance fields, when they do.

Why Not Both?

Al and I talked about both possibly being necessary - one for simple cases and the other for more complex dependencies.

Better Type-Checking

If we can get the decorator parameterized by the class type (which doesn't happen automatically, unfortunately), we can constrain the options so that they're valid keys, though not valid properties in the Polymer sense:

export function computed<T = any>(...dependencies: keyof T) { ... }

class X {
  @property() a: number;
  @property() b: number;
  @computed<X>('a', 'd') get c(): number { return this.a + this.b; }
  // error on 'd' :)
}

Al and I were also playing with deconstructing the Polymer computed property syntax, within @property():

class X {
  @property() a: number;
  @property() b: number;
  @property({
    computedFrom: ['a', 'b'],
    computed: (a: number, b: number) => a + b,
  })
  readonly c: number;
}

This is kind of nice in that it forces the computing function to be pure, since it can't access instance fields, and it keeps the computing function close to the property declaration.

OK... discuss :)

[43081j]

I was assuming lets use both to be honest.

I wrote my PR based on the assumption we would merge #18 first (because i needed computed in the property options for my PR to work, too).

As long as we can be sure that the dependencies in the getter are up to date when the getter is called (as we are referencing this rather than the passed dependencies), then i think its fine.

For any complex situations where you want splices, change objects, etc, we should just pass a function to @property if possible (rather than a function name, so we can keep strong types).

I would definitely avoid adding any more complex "handles everything" solution as we risk just blowing it out of proportion and ending up with some twisty, roundabout code.

Also totally forgot i could use keyof... maybe time to update my PR.

[aomarks]

I think I'm also leaning towards supporting both styles.

The @property style is a really straightforward implementation, has no magic, and exactly aligns with the standard Polymer style. I can't think of a great reason not to support it. You could just add a standard properties object to your element to get the same thing, since we merge decorator-created properties with an existing standard properties block if there is one.

The @computed style with getter is a nice syntax which should make most computed property use cases really concise. The only thing that irks me a little is the magic of the getter getting replaced behind the scenes. Not ideal for readability and static analysis. Still seems like a reasonable tradeoff.

We need to add options to @computed that are valid from @Property, like reflectToAttribute.

So would this look like @computed(['a', 'b'], {reflectToAttribute:true})?

[43081j]

@aomarks i think in my PR the syntax is now using spread, but yes you are pretty much right:

@computed('a', 'b', { reflectToAttribute: true })

Its a bit strange because the getter is replaced, and its no longer really what you wrote in the code. But its nicer than having the untyped link of a method name (string) to a method.

[aomarks]

#18 and #20 are merged, so both options discussed above are supported.

[yebrahim]

Can we add an example for the second way of adding a computed property (using @property) to the README, since that's the only documentation I can find?

[aomarks]

@yebrahim I've just sent our a PR with a fully updated README, which includes new documentation for defining a computed property via @property: #32. Feel free to comment!

[kito99]

Sorry I'm late to the party, but this all looks good to me. However, part of me wants to support @justinfagnani's last proposal:

class X {
  @property() a: number;
  @property() b: number;
  @property({
    computedFrom: ['a', 'b'],
    computed: (a: number, b: number) => a + b,
  })
  readonly c: number;
}

This allows us to use any function, which will make conversion from PolymerTS easier. It's also a little more straightforward since we're not defining a getter that's going to be replaced. It is more verbose and not as straightforward to code, though.

[43081j]

@kito99 if we did that, we would have to either add support for that form of computed to the polymer core, or we would have to make the @property decorator take an extended form of PropertyOptions (which could be confusing as computedFrom and co wouldn't exist in polymer).

anyhow, best to open an issue to track that wherever seems appropriate

[kito99]

Now that I've looked into things more, it's clear that the current use of the @property decorator with computed is identical to PolymerTS, which is good:

class X {
  @property() a: number;
  @property() b: number;
  @property({computed: `_c(a,b)`})
  readonly c: number;
  _c(a: number, b: number): number {
    return a + b;
  }
}

And I don't think the get() syntax will be a big problem for migration.

[kito99]

@43081j you've got a good point about support in Polymer core. I'm glad to see they now support functions for observers, but no such luck for computed properties. That being said, since we have support for a real function via the getter, I'm not sure it's all that necessary. So unless someone else really needs this, I think we're fine as-is.

[kito99]

Created pull request #35, which documents the computed property with @property.