add @computed decorator #20

43081j · 2017-11-22T21:36:06Z

This is a computed decorator (fixes #17).

The way its expected to be used is as follows:

@computed(['a', 'b'])
public get myComputedProperty() {
  return `a: ${this.a}, b: ${this.b}`;
}

It only supports simple parameters!

What this means is that splices, wildcards, length, etc. will not work. Anything which would require us to pass something into the computation function is not supported here. For that, you should create a @property with computed set manually.

To me this seems like a sacrifice worth giving so simple computations can remain as what they really are: getters. If you think otherwise, speak up and we can discuss how to support complex ones too.

43081j · 2017-11-22T23:22:32Z

@aomarks looks like this builds and tests pass now.

not sure if its the best approach though so let me know what you think.

This depends on #18. Please merge that PR first and ill rebase.

kito99 · 2017-11-28T18:52:43Z

@43081j, I've been comparing this to PolymerTS, and I can see why you took a different route. TSC never liked the PolymerTS approach of pretending a method was a property.

However, I wonder about how the developer must reference properties upon which the computed getter depends. As far as I understand it (@aomarks can correct me if I'm wrong), you can't guarantee the state of those properties if you reference them through this, which is why they are usually passed as parameters. In other words, in this example:

@computed(['dependencyOne', 'dependencyTwo'])
get computedTwo() { return this.dependencyOne + this.dependencyTwo; }

I don't believe we can guarantee that this.dependencyOne and this.dependencyTwo are in the correct state.

43081j · 2017-11-28T19:18:41Z

This implementation basically comes from my faint memory of a conversation with @justinfagnani last year, in which we both agreed a computed property is pretty much just a getter anyway. So I figured if we can at least implement the simple/common use case like one, it'll make things much nicer/smoother.

As for your question, i discussed this briefly with @rictic before making the PR and we saw no reason the properties wouldn't have updated yet. Though if anyone can correct us, i don't mind rethinking this a little.

Edit:

ah actually i wasn't thinking very well. with multiple dependencies, maybe it will be hit twice so we need some care around there. still need more feedback here from who i already cc'd.

kito99 · 2017-11-28T20:55:31Z

@43081j actually, after re-reading things, I think the properties would be initialized properly as long as they're listed in the array of properties sent to the decorator.

43081j · 2017-12-23T17:55:54Z

@aomarks @justinfagnani i changed this back to an array rather than spread... because TS doesn't like having the options parameter after a spread parameter.

justinfagnani · 2017-12-25T00:12:58Z

@43081j ah, that makes sense, but it's a bit unfortunate.

Given that, I'd say that we keep spread for @computed, and if users need to reflect to an attribute they can use @property({computed:..., reflectToAttribute: true}). @computed is really just a convenience form of @property.

43081j · 2017-12-25T11:36:34Z

@justinfagnani i agree, ive changed it back and removed the options.

43081j · 2017-12-27T20:41:48Z

rebased this onto master now too so it should be ready to merge if we're all settled on implementation design

cc @aomarks @justinfagnani

aomarks

LGTM. Let's wait for @justinfagnani before merging.

justinfagnani · 2017-12-28T21:56:18Z

src/decorators.ts

@@ -100,6 +105,29 @@ export function observe(targets: string|string[]) {
  }
 }

+/**
+ * A TypeScript accessor decorator factory that causes the decorated accessor to
+ * be called when a property changes. `targets` is either a single property


The caller should never pass a list of property names though, which I think this could be read as implying. I'd say something like "The arguments are the paths of data dependencies of the computer property, as described [here](https://www.polymer-project.org/2.0/docs/devguide/observers#define-a-computed-property)"

justinfagnani · 2017-12-28T21:58:20Z

src/decorators.ts

@@ -100,6 +105,29 @@ export function observe(targets: string|string[]) {
  }
 }

+/**
+ * A TypeScript accessor decorator factory that causes the decorated accessor to


I'd add some detail that the decorator should be applied to a getter, and that there should be no associated setter (I believe it'll be overwritten). Also, I think it's worth mentioning that unlike a plain getter, this will only be called when dependencies change, not on access, which might cause unexpected results for side-effects like logging.

justinfagnani · 2017-12-28T22:01:03Z

src/decorators.ts

+export function computed<T = any>(...targets: (keyof T)[]) {
+  return (proto: any, propName: string, descriptor: PropertyDescriptor): void => {
+    const targetString = targets.join(',');
+    const propNameCased = propName[0].toUpperCase() + propName.slice(1);


I don't think it's really necessary to name-case the property. Just add a separator before it below, like _.

justinfagnani · 2017-12-28T22:02:08Z

src/decorators.ts

+    const propNameCased = propName[0].toUpperCase() + propName.slice(1);
+    const fnName = `__compute${propNameCased}`;
+
+    proto.constructor.prototype[fnName] = descriptor.get;


proto.constructor.prototype should be unnecessary, just use proto

i had trouble using proto before but will give it another go

justinfagnani · 2017-12-28T22:03:07Z

src/decorators.ts

+    const propNameCased = propName[0].toUpperCase() + propName.slice(1);
+    const fnName = `__compute${propNameCased}`;
+
+    proto.constructor.prototype[fnName] = descriptor.get;


I think we should really use Object.defineProperty here, in case there's a name collision up the prototype chain.

justinfagnani · 2017-12-28T22:04:26Z

src/decorators.ts

+    descriptor.get = undefined;
+
+    createProperty(proto, propName, {
+      computed: `${fnName}(${targetString})`


I think it's fine to inline the targetString expression here.

googlebot · 2017-12-28T22:18:50Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

googlebot · 2017-12-28T22:19:43Z

CLAs look good, thanks!

43081j · 2017-12-28T22:46:47Z

@justinfagnani hows this? added the changes you proposed

justinfagnani

LGTM, thanks @43081j!

43081j force-pushed the computed-decorator branch 11 times, most recently from 6faffd1 to 8bc563d Compare November 22, 2017 23:21

rictic requested review from aomarks and justinfagnani November 22, 2017 23:36

justinfagnani mentioned this pull request Dec 21, 2017

Decide on how to implement computed properties #25

Closed

43081j force-pushed the computed-decorator branch 4 times, most recently from a966516 to ed3cf49 Compare December 23, 2017 17:55

43081j force-pushed the computed-decorator branch 2 times, most recently from c71ccd4 to 71d48e5 Compare December 25, 2017 11:36

add computed decorator

5ca58d9

43081j force-pushed the computed-decorator branch from 71d48e5 to 5ca58d9 Compare December 27, 2017 20:40

aomarks approved these changes Dec 27, 2017

View reviewed changes

justinfagnani suggested changes Dec 28, 2017

View reviewed changes

43081j force-pushed the computed-decorator branch from e041178 to 43104c9 Compare December 28, 2017 22:19

43081j force-pushed the computed-decorator branch 4 times, most recently from 5f3df95 to bb1971d Compare December 28, 2017 22:39

define computed getters

46ea331

43081j force-pushed the computed-decorator branch from bb1971d to 46ea331 Compare December 28, 2017 22:42

justinfagnani approved these changes Dec 28, 2017

View reviewed changes

justinfagnani merged commit f3fd7fd into Polymer:master Dec 28, 2017

43081j deleted the computed-decorator branch December 28, 2017 22:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add @computed decorator #20

add @computed decorator #20

43081j commented Nov 22, 2017 •

edited

Loading

43081j commented Nov 22, 2017 •

edited

Loading

kito99 commented Nov 28, 2017

43081j commented Nov 28, 2017 •

edited

Loading

kito99 commented Nov 28, 2017

43081j commented Dec 23, 2017

justinfagnani commented Dec 25, 2017

43081j commented Dec 25, 2017

43081j commented Dec 27, 2017

aomarks left a comment

justinfagnani Dec 28, 2017

justinfagnani Dec 28, 2017

justinfagnani Dec 28, 2017

justinfagnani Dec 28, 2017

43081j Dec 28, 2017

justinfagnani Dec 28, 2017

justinfagnani Dec 28, 2017

googlebot commented Dec 28, 2017

googlebot commented Dec 28, 2017

43081j commented Dec 28, 2017

justinfagnani left a comment

add @computed decorator #20

add @computed decorator #20

Conversation

43081j commented Nov 22, 2017 • edited Loading

43081j commented Nov 22, 2017 • edited Loading

kito99 commented Nov 28, 2017

43081j commented Nov 28, 2017 • edited Loading

kito99 commented Nov 28, 2017

43081j commented Dec 23, 2017

justinfagnani commented Dec 25, 2017

43081j commented Dec 25, 2017

43081j commented Dec 27, 2017

aomarks left a comment

Choose a reason for hiding this comment

justinfagnani Dec 28, 2017

Choose a reason for hiding this comment

justinfagnani Dec 28, 2017

Choose a reason for hiding this comment

justinfagnani Dec 28, 2017

Choose a reason for hiding this comment

justinfagnani Dec 28, 2017

Choose a reason for hiding this comment

43081j Dec 28, 2017

Choose a reason for hiding this comment

justinfagnani Dec 28, 2017

Choose a reason for hiding this comment

justinfagnani Dec 28, 2017

Choose a reason for hiding this comment

googlebot commented Dec 28, 2017

googlebot commented Dec 28, 2017

43081j commented Dec 28, 2017

justinfagnani left a comment

Choose a reason for hiding this comment

43081j commented Nov 22, 2017 •

edited

Loading

43081j commented Nov 22, 2017 •

edited

Loading

43081j commented Nov 28, 2017 •

edited

Loading