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

Bugfix/fix return type for get method #197

Merged

Conversation

kubadospial
Copy link
Contributor

references this issue: #193

@BioPhoton BioPhoton added the { } State @rx-angular/state related label Aug 2, 2020
@BioPhoton
Copy link
Member

BioPhoton commented Aug 4, 2020

@JakubDospial
Copy link
Contributor

I fount a couple of more typing issues.

See safe-pluck here: https://github.com/BioPhoton/rx-angular/blob/fix-return-type-for-get-method/libs/state/src/lib/core/utils/safe-pluck.ts
and the state (wip) here: https://github.com/BioPhoton/rx-angular/blob/fix-return-type-for-get-method/libs/state/src/lib/rx-state.service.ts

if it's not relevant with this issue please open separate issue and I will handle it on another PR

@BioPhoton
Copy link
Member

I @JakubDospial as suggested please take the propper implementation from https://github.com/BioPhoton/rx-angular/tree/fix-return-type-for-get-method.

Also please reuse the safePluck in distinctUntilSomeChanged.ts

@BioPhoton BioPhoton mentioned this pull request Aug 9, 2020
5 tasks
Copy link
Member

@hoebbelsB hoebbelsB left a comment

Choose a reason for hiding this comment

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

thanks for your great work!
I have adjusted the behavior of safePluck to fit the null undefined conventions of our other internals.

The reason why safePluck is safe is that it raises no error when a corrupt input comes by. This was the most frustrating thing when using the old pluck pipe. You always had to check for null and undefined before you could safely use it.

libs/state/src/lib/core/utils/safe-pluck.ts Outdated Show resolved Hide resolved
libs/state/src/lib/core/utils/safe-pluck.ts Outdated Show resolved Hide resolved
libs/state/spec/core/utils/safe-pluck.spec.ts Outdated Show resolved Hide resolved
kubadospial and others added 3 commits August 10, 2020 16:30
Co-authored-by: Julian Jandl <hoebbelsb@gmail.com>
Co-authored-by: Julian Jandl <hoebbelsb@gmail.com>
Co-authored-by: Julian Jandl <hoebbelsb@gmail.com>
Copy link
Member

@hoebbelsB hoebbelsB left a comment

Choose a reason for hiding this comment

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

hey jakub, together with @Karnaukhov-kh I was reviewing your PR again. We maybe plan to move the safePluck method inside the transformation-helpers. That's why we wanted to make sure it really behaves like all our other internals. When you are fine with those suggestions, please commit them 👍

libs/state/spec/core/utils/safe-pluck.spec.ts Outdated Show resolved Hide resolved
libs/state/spec/core/utils/safe-pluck.spec.ts Outdated Show resolved Hide resolved
libs/state/src/lib/core/utils/safe-pluck.ts Outdated Show resolved Hide resolved
libs/state/src/lib/core/utils/safe-pluck.ts Outdated Show resolved Hide resolved
kubadospial and others added 7 commits August 12, 2020 16:43
Co-authored-by: Julian Jandl <hoebbelsb@gmail.com>
Co-authored-by: Julian Jandl <hoebbelsb@gmail.com>
Co-authored-by: Julian Jandl <hoebbelsb@gmail.com>
Co-authored-by: Julian Jandl <hoebbelsb@gmail.com>
@BioPhoton
Copy link
Member

@hoebbelsB @Karnaukhov-kh

Is this ready for merge?

@@ -14,6 +14,7 @@ describe('safePluck', () => {

it('should return value of last key', () => {
expect(safePluck(obj, ['foo', 'bar'])).toEqual(bar);
expect(safePluck(obj, 'foo')).toEqual(obj.foo);
Copy link
Member

Choose a reason for hiding this comment

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

ty :)

@hoebbelsB
Copy link
Member

@hoebbelsB @Karnaukhov-kh

Is this ready for merge?

@BioPhoton yes from my point of view!

Copy link
Member

@hoebbelsB hoebbelsB left a comment

Choose a reason for hiding this comment

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

thanks!

@hoebbelsB hoebbelsB linked an issue Aug 14, 2020 that may be closed by this pull request
@BioPhoton BioPhoton merged commit 2ccd7a9 into rx-angular:master Aug 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
{ } State @rx-angular/state related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

get('mykey') should return the value of mykey
5 participants