Pluck operator doesn't fully check for null or undefined

[cbejensen]

If I have a source that emits a nullish value and try to use pluck, I get an error.

const source = of(null).pipe(pluck('someProp'));
source.subscribe();
// ERROR TypeError: Cannot read property 'someProp' of null

I think it would be nice if pluck checked for this and returned undefined.

const source = of(null).pipe(pluck('someProp'));
source.subscribe(console.log);
// logs: undefined

The description of this operator even says, "If a property can't be resolved, it will return undefined for that value." Seems to make sense that it would do the same for the value whose property we're checking as well.

This would be especially helpful when trying to get more deeply nested values.

interface Person {
  name: Name | null;
  pet: {
    name: Name | null;
  }
}
interface Name {
  first: string,
  last: string
}
const person: Person = { name: null, pet: { name: null } };
const petFirstName = of(person).pipe(pluck('pet', 'name', 'first'));
source.subscribe(console.log);
// logs: undefined

---

Looking at the source code, I see this:

const mapper = (x: string) => {
  let currentProp = x;
  for (let i = 0; i < length; i++) {
    const p = currentProp[props[i]];
    if (typeof p !== 'undefined') {
      currentProp = p;
    } else {
      return undefined;
    }
  }
  return currentProp;
};

Seems like it could be as simple as changing

const p = currentProp[props[i]];

to

const p = currentProp && currentProp[props[i]];

P.S. Why in the mapper function is x of type string? Wouldn't it be an object?

[anirudhvarma12]

@cartant , I would like to take a stab at it. It seems like a good first issue to contribute to RxJS.

[cartant]

@anirudhvarma12 Sure. That would be great. If you are going to fix the issue, please create a PR that contains a commit with a test that fails - i.e. a test that reproduces the problem - and a second commit with the change that fixes the problem.

[anirudhvarma12]

Understood, will share a PR with these commits.

[Totati]

Hi, this issue can be closed now: https://stackblitz.com/edit/rxjs-issue-5505?file=index.ts