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

ngrxPush pipe returns undefined, type is not correct #2888

Closed
samuelfernandez opened this issue Jan 18, 2021 · 4 comments · Fixed by #2907
Closed

ngrxPush pipe returns undefined, type is not correct #2888

samuelfernandez opened this issue Jan 18, 2021 · 4 comments · Fixed by #2907

Comments

@samuelfernandez
Copy link
Contributor

Minimal reproduction of the bug/regression with instructions:

https://stackblitz.com/edit/angular-ivy-9bbbxq?file=src/app/app.component.html

Expected behavior:

Right now the typing of the pipe is:
https://github.com/ngrx/platform/blob/master/modules/component/src/push/push.pipe.ts#L82

transform<T>(potentialObservable: ObservableInput<T>): T;

However, if the source doesn't emit on subscription, the first returned value will be undefined.

A more strict and real typing would be:

transform<T>(potentialObservable: ObservableInput<T>): T | undefined;

Versions of NgRx, Angular, Node, affected browser(s) and operating system(s):

@angular: ^11.0.8
@ngrx/component: ^10.1.2

Other information:

I would be willing to submit a PR to fix this issue

[x] Yes (Assistance is provided if you need help submitting a pull request)
[ ] No

@timdeschryver
Copy link
Member

timdeschryver commented Jan 30, 2021

Seems like a good change to me.
The Angular async pipe uses null instead of undefined tho, perhaps we should try to keep the same signature/behavior ?

@samuelfernandez
Copy link
Contributor Author

Angular team acknowledges that undefined would be a better option, but they can't make the change because it would be too breaking.

Since this is new in ngrx and there shouldn't be so many compatibility concerns as with Angular, I'd say to go with undefined instead of with null. If this becomes too opinionated we could define a configuration token with the default empty value.

Besides those comments form the Angular issues, I personally would prefer it for an additional reason. The pipe is used mainly at inputs, and having as empty value undefined would allow to simply use an optional property input:

<my-comp [value]="value$ | ngrxPush">
@Input() value?: string;

// Instead of
@Input() value: string | null;

@timdeschryver
Copy link
Member

Thanks for the clarification @samuelfernandez
Feel free to create a Pull Request.

@samuelfernandez
Copy link
Contributor Author

PR created, ready to be reviewed

brandonroberts pushed a commit that referenced this issue Feb 2, 2021
)

BREAKING CHANGE:

BEFORE:

ngrxPush typing doesn't consider `undefined` when the input type is an observable

AFTER:

ngrxPush typing considers `undefined` when the input type is an observable

Closes #2888
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants
@samuelfernandez @timdeschryver and others