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

props doesn't accept HttpErrorResponse #2892

Closed
dev054 opened this issue Jan 22, 2021 · 7 comments · Fixed by #3060
Closed

props doesn't accept HttpErrorResponse #2892

dev054 opened this issue Jan 22, 2021 · 7 comments · Fixed by #3060

Comments

@dev054
Copy link

dev054 commented Jan 22, 2021

Minimal reproduction of the bug/regression with instructions:

I want to pass the HttpError through an action and so I do the following:

export const someFail = createAction(
  '[Feature] Some fail',
  props<HttpErrorResponse>(),
);

... however it doesn't compile:

Captura de Tela 2021-01-21 às 22 03 14
Captura de Tela 2021-01-21 às 22 03 51

Expected behavior:

props accept HttpErrorResponse.

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

Ngrx 10, Angular 11.

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

@markostanimirovic
Copy link
Member

@dev054
HttpErrorResponse extends HttpResponseBase:

export abstract class HttpResponseBase {
    /**
     * Type of the response, narrowed to either the full response or the header.
     */
    readonly type: HttpEventType.Response | HttpEventType.ResponseHeader;
}

However, HttpResponseBase contains the type property, which is not allowed.

Try this:

const a = createAction('a', props<{ error: HttpErrorResponse }>());

@alex-okrushko
Copy link
Member

Thanks @markostanimirovic 👍

I wonder if we could make it clearer in the docs, e.g. don't do this (code example) and do with a prop name instead (code example).

@timdeschryver
Copy link
Member

Fyi, I'm going to steal this and going to create a lint rule for this add eslint-plugin-ngrx.

@timdeschryver
Copy link
Member

@dev054 do you think it would be more clear if we would change the error message?
If so, do you have a proposition?

@david-shortman
Copy link
Contributor

Does this succinctly describe the problem?

interface with "type" property not allowed because it conflicts with the action's type

The docsite could also note the list of illegal generic arguments for props/ the type of the creator function argument.

@timdeschryver
Copy link
Member

@david-shortman you can add a note to the docs if you still want to do that.

@david-shortman
Copy link
Contributor

I think the other problem is that the error shows up in a list of messages about overloads.
Since props is meant specifically to be used with createAction, I think it would be helpful to move the NotAllowedCheck to the props interface.

brandonroberts pushed a commit that referenced this issue Nov 2, 2021
Closes #2892 

BREAKING CHANGES:

Types for props outside an action creator is more strictly checked

BEFORE:

Usage of `props` outside of an action creator with invalid types was allowed

AFTER:

Usage of `props` outside of an action creator now breaks for invalid types
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.

6 participants