Skip to content

Commit

Permalink
feat(entity): add undefined to Dictionary's index signature (#1719)
Browse files Browse the repository at this point in the history 
BREAKING CHANGE:

Dictionary could be producing undefined but previous typings were not explicit about it.
  • Loading branch information
@alex-okrushko @brandonroberts
alex-okrushko authored and brandonroberts committed Apr 9, 2019
1 parent 0abc948 commit d472757
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 3 deletions.
2 changes: 1 addition & 1 deletion modules/entity/spec/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ ts_test_library(
),
deps = [
"//modules/entity",
"//modules/store",
"@npm//rxjs",
],
)
Expand All @@ -17,6 +18,5 @@ jasmine_node_test(
name = "test",
deps = [
":test_lib",
"//modules/entity",
],
)
9 changes: 9 additions & 0 deletions modules/entity/spec/state_selectors.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
AnimalFarm,
TheGreatGatsby,
} from './fixtures/book';
import { MemoizedSelector, createSelector } from '@ngrx/store';

describe('Entity State Selectors', () => {
describe('Composed Selectors', () => {
Expand Down Expand Up @@ -89,6 +90,14 @@ describe('Entity State Selectors', () => {
expect(entities).toEqual(state.entities);
});

it('should type single entity from Dictionary as entity type or undefined', () => {
// MemoizedSelector acts like a type checker
const singleEntity: MemoizedSelector<
EntityState<BookModel>,
BookModel | undefined
> = createSelector(selectors.selectEntities, enitites => enitites[0]);
});

it('should create a selector for selecting the list of models', () => {
const models = selectors.selectAll(state);

Expand Down
4 changes: 2 additions & 2 deletions modules/entity/src/models.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,11 @@ export type IdSelectorNum<T> = (model: T) => number;
export type IdSelector<T> = IdSelectorStr<T> | IdSelectorNum<T>;

export interface DictionaryNum<T> {
[id: number]: T;
[id: number]: T | undefined;
}

export abstract class Dictionary<T> implements DictionaryNum<T> {
[id: string]: T;
[id: string]: T | undefined;
}

export interface UpdateStr<T> {
Expand Down

4 comments on commit d472757

@mtaran-google
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey Alex, do you have any more info on what exactly are/were the places where the EntityStore is/was expected to be storing values that are undefined?

@alex-okrushko
Copy link
Member Author

Choose a reason for hiding this comment

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

@mtaran-google generally accessing entity by index could return undefined.

@mtaran-google
Copy link
Contributor

Choose a reason for hiding this comment

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

You're right that querying the entity dictionary by index can potentially return undefined, but I don't think that's sufficient reason to add | undefined in these types. To me, this seems to mirror the issue at the core of this TypeScript FR, and this comment really sums up the problems that this typing results in. TL;DR: It adds extra warnings both where they're useful and where they're not, which results in the user learning to ignore these kinds of type errors, leading to them not really being any more helpful than if they were absent.

@alex-okrushko
Copy link
Member Author

Choose a reason for hiding this comment

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

Dictionary is returned when you do selectEntities - which are meant to be accessed by index. They should have checks for undefined.
If you need the entire collection as an Array you could use selectAll and then the T[] would be returned - without undefined types.

Please sign in to comment.