Skip to content
This repository has been archived by the owner on Apr 20, 2018. It is now read-only.

Implement deepPluck, a pluck operator for nested properties #526

Merged
merged 1 commit into from
Feb 5, 2015

Conversation

sergi
Copy link
Contributor

@sergi sergi commented Jan 27, 2015

I found myself implementing this operator time and again in my projects, since it is quite convenient. I thought it would be great to have it in the main RxJS distribution.

deepPluck is like pluck for nested properties. This would be an example of use:

var source = Rx.Observable
    .from([
        { valueA: { valueB: { valueC: 0 }}},
        { valueA: { valueB: { valueC: 1 }}},
        { valueA: { valueB: 2 }},
    ])
    .deepPluck('valueA', 'valueB', 'valueC');

var subscription = source.subscribe(
    function (x) {
        console.log('Next: ' + x);
    },
    function (err) {
        console.log('Error: ' + err);
    },
    function () {
        console.log('Completed');
    });

// => Next: 0
// => Next: 1
// => Next: undefined
// => Completed

deepPluck will return undefined for any property that can't be resolved, just like pluck does.

I included the rx generated files in the PR, please let me know if that's wrong.

@urmastalimaa
Copy link
Contributor

I think it's worth mentioning that this implementation differs from .pluck('valueA').pluck('valueB').pluck('valueC')
by not raising a TypeError on missing properties in the middle of the chain.

I'd suggest simplifying the property resolving by using:

return args.reduce(function(resolvedProperty, propertyName) {
  return resolvedProperty === undefined && resolvedProperty || resolvedProperty[propertyName];
}, x);

By the way, do not include files from the dist folder in the PR.

@urmastalimaa
Copy link
Contributor

Also the operator could live in it's own file (modify Gruntfile.js to include).
It's also useful to include falsy property values like 0 or false in the tests (the current implementation is faulty with respect to them).

@urmastalimaa
Copy link
Contributor

Scratch the comment about using Array.reduce, it's not supported in IE < 9.

@mattpodwysocki
Copy link
Member

@sergi my concern with that is with pluck and why it's not implemented like that in any other library such as underscore, lo-dash, etc is the following:

λ node
> var foo = {};
undefined
> foo['.bar'] = 42;
42
> foo
{ '.bar': 42 }

As you can see, having a dot in front of a property name is perfectly valid, so by doing a deep pluck, I'd be missing said properties.

@urmastalimaa
Copy link
Contributor

@mattpodwysocki The proposed deepPluck operator used a list of properties not a string to be evaluated, so having dots shouldn't be an issue.

@sergi sergi force-pushed the deep_pluck branch 3 times, most recently from 9987448 to 04a66b2 Compare January 27, 2015 22:19
@sergi
Copy link
Contributor Author

sergi commented Jan 27, 2015

@urmastalimaa I made the changes you proposed, made sense. I thought of using reduce at first too but because of compatibility and the recent focus on performance I decided to go with the good old loop.

@mattpodwysocki Yeah my first thought was doing it based on strings, but there were too many edge cases. As @urmastalimaa points out, it now takes n parameters that represent nested properties.

@urmastalimaa
Copy link
Contributor

@sergi The implementation is still buggy

Rx.Observable.return({a: 0}).deepPluck('a').subscribe(console.log.bind(console))
// => undefined

I suggest adding a falsy value to the tests.

@sergi
Copy link
Contributor Author

sergi commented Jan 27, 2015

@urmastalimaa Thanks for the pointer, solved now.

@paulpdaniels
Copy link
Contributor

Minor micro-optimization I'll put out there as well, the length of the argument array can be pre-cached to avoid having it called (# of elements) * (# of arguments) times.

Shouldn't really matter in Chrome and Firefox, but this will really bite you if you are using IE.

@sergi
Copy link
Contributor Author

sergi commented Jan 28, 2015

@paulpdaniels thanks for the suggestion, I implemented it.

@mattpodwysocki
Copy link
Member

@sergi @paulpdaniels should we just use this in place of pluck because technically there are no breaking changes to the API at all, just adding to it.

@paulpdaniels
Copy link
Contributor

I'm for it.

@sergi
Copy link
Contributor Author

sergi commented Feb 4, 2015

@mattpodwysocki That makes sense. If everybody agrees I can update it accordingly then.

@mattpodwysocki
Copy link
Member

@sergi I think it's agreed, DO IT FOR THE GREATER GOOD!

@sergi
Copy link
Contributor Author

sergi commented Feb 5, 2015

@mattpodwysocki lol, done :)

mattpodwysocki added a commit that referenced this pull request Feb 5, 2015
Implement `deepPluck`, a pluck operator for nested properties
@mattpodwysocki mattpodwysocki merged commit 8364c13 into Reactive-Extensions:master Feb 5, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants