feat(entity): add undefined to Dictionary's index signature

[alex-okrushko]

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/ngrx/platform/blob/master/CONTRIBUTING.md#commit
• Tests for the changes have been added (for bug fixes / features)
• Documentation has been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Index signatures always return value and never return undefined 🤯

export class Dictionary {
  [id: string]: number;
}

const dict = new Dictionary();
dict[100] // <--- type is number

microsoft/TypeScript#13778

Typescript is telling me that I don't need to check because it's of type number. In reality it's actually number | undefined, and I should check for the undefined.

Suggestion in the bug from TS team is to add " | undefined at their definition sites".

[ ] Bugfix
[x] 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?

This became particular painful when MemoizedSelector<State, Result> started to validate the return type of the selector to make sure it's actually Result.

Closes #

What is the new behavior?

If one relied on Dictionary<Entity> to not return undefined they would have to adjust the code.

Does this PR introduce a breaking change?

Yes, it is. See 👆

[x] Yes
[ ] No

Other information

[ngrxbot]

Preview docs changes for 8c1badb at https://previews.ngrx.io/pr1719-8c1badb/

[ngrxbot]

Preview docs changes for 5666a94 at https://previews.ngrx.io/pr1719-5666a94/

[ngrxbot]

Preview docs changes for 8ea9a63 at https://previews.ngrx.io/pr1719-8ea9a63/

[brandonroberts]

What message should be displayed in the breaking change?

[alex-okrushko]

What message should be displayed in the breaking change?

message in the announcement or message in the error?

Dictionary could be producing undefined but previous typings were not explicit about it.

[larrifax]

I know I'm a bit late to the party here, but I just wanted to chime in and say that this change makes it harder to create selectors that does something more than just picking a specific index out of the dictionary.

As pointed out in the Typescript-issue mentioned in the PR description (microsoft/TypeScript#13778), Typescript isn't smart enough to remove undefined from the type when mapping over the Dictionary, which is why the Typescript team hasn't made this change in their base libs.

This means that undefined is included both in the type sent in to the mapping function as well as the return type of ramda's or lodash's pickBy, values, and I guess also Object.values / Object.entries.

Seems like we have to choose between two evils at the time being..

[alex-okrushko]

This means that undefined is included both in the type sent in to the mapping function as well as the return type of ramda's or lodash's pickBy, values, and I guess also Object.values / Object.entries.

@larrifax That's correct. In cases like that I either add filter((value): value is NonNullable<typeof value> => !!value) or sometime assertions (value!) if I'm looping over myself.

Also, if you need all values then selectAll will return an Array of that type without undefined.