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

fix(pluck): operator breaks with null/undefined inputs. #5524

Merged
merged 3 commits into from
Jun 29, 2020

Conversation

anirudhvarma12
Copy link
Contributor

Description:
Add a fix to prevent the pluck operator from breaking on null or undefined inputs.

Related issue (if exists):
#5505

@anirudhvarma12 anirudhvarma12 changed the title fixes #5505: Pluck operator breaks with null/undefined inputs. fix(pluck): operator breaks with null/undefined inputs. Jun 23, 2020
Copy link
Collaborator

@cartant cartant left a comment

Choose a reason for hiding this comment

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

Looks good, but I don't think the nullish coalescing operator should be used here.

src/internal/operators/pluck.ts Outdated Show resolved Hide resolved
@anirudhvarma12
Copy link
Contributor Author

@cartant , is there a process to PR getting merged after approval? (Sorry if this is already documented somewhere, I could not find it)

@cartant
Copy link
Collaborator

cartant commented Jun 26, 2020

PRs that update actual library code - i.e. not just tests or docs - need to be approved and merged by Ben.

@benlesh benlesh merged commit c5f6550 into ReactiveX:master Jun 29, 2020
benlesh pushed a commit that referenced this pull request Jun 29, 2020
* test(pluck): add failing test case for using null value.

* fix(pluck): check for null/undefined object before attempting to access prop

* Remove null coalescing when checking values to prevent null values being converted to undefined
@benlesh
Copy link
Member

benlesh commented Jun 29, 2020

Merged also into 6.x, although with a slight alteration, as we don't compile with TS 3.9 there.

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.

3 participants