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.

Sign up for GitHub

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

Implement upsertOne() and upsertMany() for entity adapters #550

Closed
wants to merge 13 commits into from
Closed

Implement upsertOne() and upsertMany() for entity adapters #550

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
46b62b9
Improve performance for sorted entity adapter
Sep 29, 2017
6d2999c
Remove updateManyMutably() functionality from merge() in sorted_state…
Sep 29, 2017
ee9067c
Fix return value from addAllMutably() in sorted_state_adapter
Sep 29, 2017
8dcb54c
Improve performance of collection methods for unsorted state adapter
Sep 29, 2017
8efba70
Fix for unsupported includes() in unsorted_state_adapter
Sep 29, 2017
2b9cc50
Expose effectMetadata() useful in testing
Oct 17, 2017
848e94f
Merge branch 'master' of https://github.com/ngrx/platform
Oct 17, 2017
ba2eaad
Merge branch 'master' of github.com:ngrx/platform
Oct 17, 2017
f06e744
Shorter syntax for effects metadata
Oct 17, 2017
0673d12
Fix docs for getEffectsMetadata()
Oct 17, 2017
1bc563e
Add key type checking for EffectsMetadata
Oct 17, 2017
cb0bee9
Merge https://github.com/ngrx/platform
Nov 6, 2017
9826793
Implement upsertOne() and upsertMany()
Nov 6, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 68 additions & 0 deletions modules/entity/spec/sorted_state_adapter.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,4 +231,72 @@ describe('Sorted State Adapter', () => {
},
});
});

it('should let you add one entity to the state with upsert()', () => {
const withOneEntity = adapter.upsertOne(
{
id: TheGreatGatsby.id,
changes: TheGreatGatsby,
},
state
);

expect(withOneEntity).toEqual({
ids: [TheGreatGatsby.id],
entities: {
[TheGreatGatsby.id]: TheGreatGatsby,
},
});
});

it('should let you update an entity in the state with upsert()', () => {
const withOne = adapter.addOne(TheGreatGatsby, state);
const changes = { title: 'A New Hope' };

const withUpdates = adapter.upsertOne(
{
id: TheGreatGatsby.id,
changes,
},
withOne
);

expect(withUpdates).toEqual({
ids: [TheGreatGatsby.id],
entities: {
[TheGreatGatsby.id]: {
...TheGreatGatsby,
...changes,
},
},
});
});

it('should let you upsert many entities in the state', () => {
const firstChange = { title: 'Zack' };
const secondChange = { title: 'Aaron' };
const withMany = adapter.addAll([TheGreatGatsby], state);

const withUpserts = adapter.upsertMany(
[
{ id: TheGreatGatsby.id, changes: firstChange },
{ id: AClockworkOrange.id, changes: secondChange },
],
withMany
);

expect(withUpserts).toEqual({
ids: [AClockworkOrange.id, TheGreatGatsby.id],
entities: {
[TheGreatGatsby.id]: {
...TheGreatGatsby,
...firstChange,
},
[AClockworkOrange.id]: {
...AClockworkOrange,
...secondChange,
},
},
});
});
});
Copy link

Choose a reason for hiding this comment

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

in upsert we pass list of entities to be added if missing or updates if exists.
in your implementation - if an entity already exists will it get updated even if it is the same as the original element?
if the answer is yes - this raises a performance question, as entities that have no changes would get new references and reload in components with OnPush change detection strategy.

anyway whatever the answer is - perhaps a test that clarifies it would be a good idea:
so if an existing entity gets an update with no changes -
a) if the functionality is to recreate it the test will show that a new entity was created.
b) if the functionality is to ignore it the test will show that no new entity was created.

68 changes: 68 additions & 0 deletions modules/entity/spec/unsorted_state_adapter.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,4 +202,72 @@ describe('Unsorted State Adapter', () => {
},
});
});

it('should let you add one entity to the state with upsert()', () => {
const withOneEntity = adapter.upsertOne(
{
id: TheGreatGatsby.id,
changes: TheGreatGatsby,
},
state
);

expect(withOneEntity).toEqual({
ids: [TheGreatGatsby.id],
entities: {
[TheGreatGatsby.id]: TheGreatGatsby,
},
});
});

it('should let you update an entity in the state with upsert()', () => {
const withOne = adapter.addOne(TheGreatGatsby, state);
const changes = { title: 'A New Hope' };

const withUpdates = adapter.upsertOne(
{
id: TheGreatGatsby.id,
changes,
},
withOne
);

expect(withUpdates).toEqual({
ids: [TheGreatGatsby.id],
entities: {
[TheGreatGatsby.id]: {
...TheGreatGatsby,
...changes,
},
},
});
});

it('should let you upsert many entities in the state', () => {
const firstChange = { title: 'First Change' };
const secondChange = { title: 'Second Change' };
const withMany = adapter.addAll([TheGreatGatsby], state);

const withUpserts = adapter.upsertMany(
[
{ id: TheGreatGatsby.id, changes: firstChange },
{ id: AClockworkOrange.id, changes: secondChange },
],
withMany
);

expect(withUpserts).toEqual({
ids: [TheGreatGatsby.id, AClockworkOrange.id],
entities: {
[TheGreatGatsby.id]: {
...TheGreatGatsby,
...firstChange,
},
[AClockworkOrange.id]: {
...AClockworkOrange,
...secondChange,
},
},
});
});
});
3 changes: 3 additions & 0 deletions modules/entity/src/models.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,9 @@ export interface EntityStateAdapter<T> {

updateOne<S extends EntityState<T>>(update: Update<T>, state: S): S;
updateMany<S extends EntityState<T>>(updates: Update<T>[], state: S): S;

upsertOne<S extends EntityState<T>>(update: Update<T>, state: S): S;
upsertMany<S extends EntityState<T>>(updates: Update<T>[], state: S): S;
}

export type EntitySelectors<T, V> = {
Expand Down
28 changes: 28 additions & 0 deletions modules/entity/src/sorted_state_adapter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,32 @@ export function createSortedStateAdapter<T>(selectId: any, sort: any): any {
return merge(models, state);
}

function upsertOneMutably(update: Update<T>, state: R): boolean;
function upsertOneMutably(update: any, state: any): boolean {
return upsertManyMutably([update], state);
}

function upsertManyMutably(updates: Update<T>[], state: R): boolean;
function upsertManyMutably(updates: any[], state: any): boolean {
const added: T[] = [];
const updated: Update<T>[] = [];

for (let index in updates) {
const update = updates[index];
if (update.id in state.entities) {
updated.push(update);
} else {
added.push({
...update.changes,
id: update.id,
});
}
}

const didMutate = updateManyMutably(updated, state);
return addManyMutably(added, state) || didMutate;
}

function merge(models: T[], state: R): boolean;
function merge(models: any[], state: any): boolean {
if (models.length === 0) {
Expand Down Expand Up @@ -123,8 +149,10 @@ export function createSortedStateAdapter<T>(selectId: any, sort: any): any {
removeAll,
addOne: createStateOperator(addOneMutably),
updateOne: createStateOperator(updateOneMutably),
upsertOne: createStateOperator(upsertOneMutably),
addAll: createStateOperator(addAllMutably),
addMany: createStateOperator(addManyMutably),
updateMany: createStateOperator(updateManyMutably),
upsertMany: createStateOperator(upsertManyMutably),
};
}
28 changes: 28 additions & 0 deletions modules/entity/src/unsorted_state_adapter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,13 +112,41 @@ export function createUnsortedStateAdapter<T>(selectId: IdSelector<T>): any {
return didMutate;
}

function upsertOneMutably(update: Update<T>, state: R): boolean;
function upsertOneMutably(update: any, state: any): boolean {
return upsertManyMutably([update], state);
}

function upsertManyMutably(updates: Update<T>[], state: R): boolean;
function upsertManyMutably(updates: any[], state: any): boolean {
const added: T[] = [];
const updated: Update<T>[] = [];

for (let index in updates) {
const update = updates[index];
if (update.id in state.entities) {
updated.push(update);
} else {
added.push({
...update.changes,
id: update.id,
Copy link
Contributor Author

@dinvlad dinvlad Nov 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

This line is problematic: if selectId != entity => entity.id, this insert will fail to create an entity with the same effective id. We may need to change the signature of the method to (updates: { id: string | number; changes: T}[], state: R), per #421 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Let's make this change.

});
}
}

const didMutate = updateManyMutably(updated, state);
return addManyMutably(added, state) || didMutate;
}

return {
removeAll,
addOne: createStateOperator(addOneMutably),
addMany: createStateOperator(addManyMutably),
addAll: createStateOperator(addAllMutably),
updateOne: createStateOperator(updateOneMutably),
updateMany: createStateOperator(updateManyMutably),
upsertOne: createStateOperator(upsertOneMutably),
upsertMany: createStateOperator(upsertManyMutably),
removeOne: createStateOperator(removeOneMutably),
removeMany: createStateOperator(removeManyMutably),
};
Expand Down