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

Make computed.or and computed.and support property expansion #13274

Merged
merged 5 commits into from Apr 10, 2016

Conversation

Projects
None yet
2 participants
@ming-codes
Contributor

ming-codes commented Apr 7, 2016

computed.or and computed.and already supports more than two properties. I added some test cases for this use case.

Added code to pass getProperties through Ember.expandProperties to further enhance the above case.

Fix jshint error
Extract the extraction function out to fix jshint error
@ming-codes

This comment has been minimized.

Show comment
Hide comment
@ming-codes

ming-codes Apr 8, 2016

Contributor

@rwjblue All tests passed

Contributor

ming-codes commented Apr 8, 2016

@rwjblue All tests passed

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Apr 8, 2016

Member

Looks good, can you update the docs for computed.or and computed.and to add an example using brace expansion and also squash commits?

Member

rwjblue commented Apr 8, 2016

Looks good, can you update the docs for computed.or and computed.and to add an example using brace expansion and also squash commits?

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Apr 8, 2016

Member

Hmm, actually, I just had a thought. It seems like we should call expandProperties upon creation of the macro (inside generateComputedWithProperties around here). The current implementation seems like it would be calling expandProperties on every dependent key invalidation (which seems like a bit of wasted work).

Member

rwjblue commented Apr 8, 2016

Hmm, actually, I just had a thought. It seems like we should call expandProperties upon creation of the macro (inside generateComputedWithProperties around here). The current implementation seems like it would be calling expandProperties on every dependent key invalidation (which seems like a bit of wasted work).

@ming-codes

This comment has been minimized.

Show comment
Hide comment
@ming-codes

ming-codes Apr 10, 2016

Contributor

computed.and doesn't seem to follow the JavaScript-like behavior of computed.or. As in, computed.and always coerce to false rather than the original DK's falsy value. I wonder if this is a bug so I can fix it.

Test case below

  var obj = { one: true, two: true };
  defineProperty(obj, 'oneAndTwo', and('one', 'two'));

  set(obj, 'one', null);
  set(obj, 'two', 'Yes');

  equal(get(obj, 'oneAndTwo'), null, 'returns falsy value as in &&'); // expects `null` as JavaScript expression `null && 'Yes'` eval to `null`
Contributor

ming-codes commented Apr 10, 2016

computed.and doesn't seem to follow the JavaScript-like behavior of computed.or. As in, computed.and always coerce to false rather than the original DK's falsy value. I wonder if this is a bug so I can fix it.

Test case below

  var obj = { one: true, two: true };
  defineProperty(obj, 'oneAndTwo', and('one', 'two'));

  set(obj, 'one', null);
  set(obj, 'two', 'Yes');

  equal(get(obj, 'oneAndTwo'), null, 'returns falsy value as in &&'); // expects `null` as JavaScript expression `null && 'Yes'` eval to `null`
@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Apr 10, 2016

Member

Yes, I believe that it should behave the same as computed.or (and normal JS)

Member

rwjblue commented Apr 10, 2016

Yes, I believe that it should behave the same as computed.or (and normal JS)

@ming-codes

This comment has been minimized.

Show comment
Hide comment
@ming-codes

ming-codes Apr 10, 2016

Contributor

@rwjblue Done. I think github let you squash and merge now. Or I can do it. Let me know.

Contributor

ming-codes commented Apr 10, 2016

@rwjblue Done. I think github let you squash and merge now. Or I can do it. Let me know.

@rwjblue rwjblue merged commit fad3662 into emberjs:master Apr 10, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Apr 10, 2016

Member

@ming-codes thank you!

Member

rwjblue commented Apr 10, 2016

@ming-codes thank you!

toddjordan added a commit to toddjordan/ember.js that referenced this pull request Sep 9, 2016

Make computed.or and computed.and support property expansion (emberjs…
…#13274)

* computed property support property expansion

* move extraction to design time

* computed.and should behave like JavaScript

* updated doc to show brace expansion
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment