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(Example): linter problems #1597

Merged
merged 3 commits into from
Mar 5, 2019
Merged

Conversation

tja4472
Copy link
Contributor

@tja4472 tja4472 commented Mar 3, 2019

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

vscode reports Typescript problems.

Closes #1592

What is the new behavior?

No problems.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@coveralls
Copy link

coveralls commented Mar 3, 2019

Coverage Status

Coverage increased (+0.002%) to 89.515% when pulling 8a1d2c2 on tja4472:issue-1592 into 5cb9a46 on ngrx:master.

@ngrxbot
Copy link
Collaborator

ngrxbot commented Mar 3, 2019

Preview docs changes for 402bf55 at https://previews.ngrx.io/pr1597-402bf55/

Copy link
Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

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

What about adding the ngrx-store-freeze path in at https://github.com/ngrx/platform/blob/master/tsconfig.json#L37 ?

We don't have to add another tsconfig.json I think.

@tja4472
Copy link
Contributor Author

tja4472 commented Mar 3, 2019

Unfortunately this file has "strict": true which is also a problem for example-app.

@timdeschryver
Copy link
Member

What's the problem with strict: true?

@tja4472
Copy link
Contributor Author

tja4472 commented Mar 3, 2019

Adding the following to tsconfig.app.json

    "strictPropertyInitialization": false,
    "strict": true,

and building gives

ERROR in projects/example-app/src/app/reducers/index.ts(42,3): error TS2322: Type '(state: State | undefined, action: LayoutActionsUnion) => State' is not assignable to type 'ActionReducer<State, Action>'.
  Types of parameters 'action' and 'action' are incompatible.
    Type 'Action' is not assignable to type 'LayoutActionsUnion'.
      Type 'Action' is not assignable to type 'CloseSidenav'.
        Types of property 'type' are incompatible.
          Type 'string' is not assignable to type 'LayoutActionTypes.CloseSidenav'.
projects/example-app/src/app/reducers/index.ts(48,3): error TS2322: Type '(state: State, action: any) => any' is not assignable to type 'ActionReducer<State, Action>'.
  Types of parameters 'state' and 'state' are incompatible.
    Type 'State | undefined' is not assignable to type 'State'.
      Type 'undefined' is not assignable to type 'State'.
projects/example-app/src/app/core/containers/app.component.ts(47,38): error TS2345: Argument of type '(source$: Observable<State>) => Observable<boolean>' is not assignable to parameter of type 'OperatorFunction<State, boolean>'.
  Types of parameters 'source$' and 'source' are incompatible.
    Type 'import("C:/VSCode/2019/git-hub/platform/node_modules/rxjs/internal/Observable").Observable<import("C:/VSCode/2019/git-hub/platform/projects/example-app/src/app/reducers/index").State>' is not assignable to type 'import("C:/VSCode/2019/git-hub/platform/node_modules/rxjs/internal/Observable").Observable<import("C:/VSCode/2019/git-hub/platform/projects/example-app/src/app/auth/reducers/index").State>'.
      Type 'import("C:/VSCode/2019/git-hub/platform/projects/example-app/src/app/reducers/index").State' is not assignable to type 'import("C:/VSCode/2019/git-hub/platform/projects/example-app/src/app/auth/reducers/index").State'.
        Property 'auth' is missing in type 'State'.
projects/example-app/src/app/books/containers/selected-book-page.component.ts(29,5): error TS2322: Type 'Observable<boolean | "" | null>' is not assignable to type 'Observable<boolean>'.
  Type 'boolean | "" | null' is not assignable to type 'boolean'.
    Type 'null' is not assignable to type 'boolean'.

@timdeschryver
Copy link
Member

🤦‍♂️ Doh, I thought this was just about `ngrx-store-freeze.

I would still like to have strict on.

As a workaround, we can solve the typing issue as #951 (comment)

And rewrite the logger as

export function logger(reducer: ActionReducer<State>): ActionReducer<State> {
  return (state, action) => {
    const result = reducer(state, action);
    console.groupCollapsed(action.type);
    console.log('prev state', state);
    console.log('action', action);
    console.log('next state', result);
    console.groupEnd();

    return result;
  };
}

Thoughts?

@tja4472
Copy link
Contributor Author

tja4472 commented Mar 3, 2019

@tja4472 tja4472 changed the title fix(example-app): add tsconfig.json fix(example-app): work with strict = true Mar 4, 2019
@ngrxbot
Copy link
Collaborator

ngrxbot commented Mar 4, 2019

Preview docs changes for 8a1d2c2 at https://previews.ngrx.io/pr1597-8a1d2c2/

Copy link
Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@timdeschryver timdeschryver changed the title fix(example-app): work with strict = true fix(Example): linter problems Mar 5, 2019
@timdeschryver timdeschryver merged commit 4cfcc08 into ngrx:master Mar 5, 2019
@timdeschryver
Copy link
Member

Thanks @tja4472 !

tja4472 added a commit to tja4472/platform that referenced this pull request Mar 20, 2019
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.

example-app: vscode reports Typescript problems
4 participants