-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[FEATURE computed-property-syntax-new] #9477
Conversation
test('writable - setting property invokes "set" fn', function () { | ||
o.set('half', 11); | ||
equal(o.get('a'), 5.5, 'updates dependant properties #1'); | ||
equal(o.get('b'), 5.5, 'updates dependant properties #2'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self:
var vals = o.getProperties(['a', 'b']);
deepEqual(vals, {a: 5.5, b: 5.5}, 'Setting computed property updates dependent properties as expected');
I'm sorry I missed the RFC (emberjs/rfcs#11). I don't know if it's too late to provide feedback or where I should put it, but one thing that occurs to me is that often times my CPs that have complicated logic look like this: Ember.computed('a', 'b', 'c', 'd', function (key, val) {
if (arguments.length === 2) {
// COMPLICATED "SET" LOGIC
}
// RETURN COMPLICATED "GET" LOGIC
}) In this case I'm effectively doing a "set", followed by a "get", there isn't a distinct code path for building the return value that's based on the type of operation that's taking place. I can just imagine a situation with computed-property-syntax-new where some amount of complicated logic that is necessary to build a return value is duplicated between getters and setters. I almost wonder if set should not return a value and instead after |
oh man, awesome! I will find some time today to review. |
We stepped into each other!! I came up with this simple solution a moment ago: #9478 |
We need to ensure super works for get and set correctly. I have some internals ideas I will provide once I can review this |
@stefanpenner looking forward to exchanging ideas. I'll add a test coverage for calling super on get and set |
👍 Really like this new syntax, can't wait for it to land. |
Conflicts: features.json
sorry for letting this linger, I have assigned myself to review. Can you also rebase? |
@stefanpenner PR #9527 is a duplicated of this one. Check it too. |
@stefanpenner can this be closed considering #9527 was merged? |
Yes, I believe so. |
RFC: emberjs/rfcs#11
This feature adds some optional new syntax for defining computed properties. Current methods for building computed properties still work fine, but now we can pass an object instead of a function
So, here's a concrete example of a "writable" macro with the current syntax
And the equivalent with the proposed syntax.