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

[FEATURE uniqBy] Adds Enumerable.uniqBy and Computed.uniqBy #12875

Merged
merged 1 commit into from
Apr 9, 2016

Conversation

seanjohnson08
Copy link
Contributor

ORIGINAL PR: #12758

Almost all of the enumerable methods that serve to produce some new result from a list follow the following pattern:

Enumerate by object Enumerate by object property
filter filterBy
map mapBy
find findBy
reject rejectBy
uniq ????

Except uniq! My proposal is to add a uniqBy method to both the Enumerable class and to the computed reduce macros to allow the user to reduce a list down to its unique values given some key. A prime example is if I'm dealing with a list of records, or objects returned that have a primary key:

var records = Ember.A([ {id: 1, name: 'Adam' }, {id: 2, name: 'Eve' }, {id: 1, name: 'Adam'} ]);

If I wanted to remove the duplicate records, I would then be able to use the result of records.uniqBy('id') to get a list of just the unique records in the list.

This would not alter or modify the existing uniq methods, it would just serve to fill in a blank.

@@ -363,6 +364,47 @@ export function uniq(...args) {
}

/**
A computed property which returns a new array with all the unique
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rwjblue im assuming this code/docs also needs to be feature flagged?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the docs should be inside the feature flags.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@mixonic
Copy link
Member

mixonic commented Jan 31, 2016

Thanks for the continued polish on this @seanjohnson08 :-)

@rwjblue
Copy link
Member

rwjblue commented Jan 31, 2016

We should also ensure that a module is added to ember-cli-shims when Ember.computed.uniqBy is present.

@seanjohnson08
Copy link
Contributor Author

@mixonic thanks for all of the feedback/help. One last issue: When I flag off Enumerable.uniqBy - it isn't available in the testing environment. Anything special I need to do?

@rwjblue
Copy link
Member

rwjblue commented Jan 31, 2016

You have to also flag the new tests to only run when the feature is enabled. Then you can check the checkbox in the test runner environment for "enable optional features".

@seanjohnson08
Copy link
Contributor Author

Alright - so I've moved the tests to the right place, but I don't believe I'm flagging off the tests correctly. They all pass locally (with enableoptionalfeatures=true) and don't run with enableoptionalfeatures=false, but they're failing the Travis build tests.

}
});

// QUnit.test('uniqBy is readOnly', function() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be removed, I expect.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I uncommented this out only because it was messing with my editor's formatting, I've added it back. Good catch. :)

@rwjblue
Copy link
Member

rwjblue commented Jan 31, 2016

I believe the current failures are due to caching of glimmer-engine, which should have been fixed by #12882. Can you rebase?

Also, before merging we will need to squash these commits down to a single commit with a prefix of [FEATURE ember-runtime-computed-uniq-by].

@seanjohnson08
Copy link
Contributor Author

@rwjblue - rebased and squashed.

@seanjohnson08
Copy link
Contributor Author

should be good to go now - thanks for all of your help/feedback. 👍

@homu
Copy link
Contributor

homu commented Feb 5, 2016

☔ The latest upstream changes (presumably #12575) made this pull request unmergeable. Please resolve the merge conflicts.

@seanjohnson08
Copy link
Contributor Author

@rwjblue - I've resolved conflicts, any further discussion on the fate of this PR?

@rwjblue
Copy link
Member

rwjblue commented Feb 5, 2016

Should be good, will review one last time...

@tennisonchan
Copy link

+1. It would be a good computed feature to have.

@homu
Copy link
Contributor

homu commented Feb 21, 2016

☔ The latest upstream changes (presumably #12991) made this pull request unmergeable. Please resolve the merge conflicts.

@seanjohnson08
Copy link
Contributor Author

I've once again rebased on latest changes - the latest commit included @mmun's WeakMap work so I made good use of it here as well.

@mmun
Copy link
Member

mmun commented Feb 22, 2016

@seanjohnson08 WeakMap is not intended to be used as a short-lived object. This leaks memory. Please see the comment at

* There is a small but important caveat. This implementation assumes that the
* weak map will live longer (in the sense of garbage collection) than all of its
* keys, otherwise it is possible to leak the values stored in the weak map. In
* practice, most use cases satisfy this limitation which is why it is included
* in ember-metal.
.

Fortunately the fix is simple. Just use a new EmptyObject() (https://github.com/emberjs/ember.js/blob/4a404de21d70b780f2ac85b6fb265e037998d2d1/packages/ember-metal/lib/empty_object.js) instead of a WeakMap or {}. You should use a dictionary in both of

  • packages/ember-runtime/lib/computed/reduce_computed_macros.js
  • packages/ember-runtime/lib/mixins/enumerable.js

@seanjohnson08
Copy link
Contributor Author

@mmun - thanks for that insight. I have switched it back to using just a regular old object. Any closure on this PR? I hate to be a pester, but I'd rather not have to keep rebasing onto the latest changes to keep @homu happy. :)

@homu
Copy link
Contributor

homu commented Mar 19, 2016

☔ The latest upstream changes (presumably #13100) made this pull request unmergeable. Please resolve the merge conflicts.

@mixonic
Copy link
Member

mixonic commented Apr 8, 2016

@seanjohnson08 this is very much still a go. Can I ask you for a fresh rebase? I'm checking through the implementation again, but we are likely good to go!!

export function uniqBy(dependentKey, propertyKey) {
return computed(`${dependentKey}.[]`, function() {
var uniq = emberA();
var seen = {};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't @mmun asking that this use EmptyObject instead of {}? Can we do that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've rebased and switched to EmptyObject. Thanks!

@rwjblue
Copy link
Member

rwjblue commented Apr 9, 2016

Thank you @seanjohnson08!

@acorncom acorncom mentioned this pull request May 24, 2016
8 tasks
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

Successfully merging this pull request may close these issues.

7 participants