ngrx-data delete error

[chobijaeyu]

Goods day.
I've been stuck here for a few days.And i really need some help.
I'm trying to use ngrx/data,it's working fine when add,update,get. but it gets error when delete.(not 100% happen)

Minimal reproduction of the bug/regression with instructions:

fontend:https://github.com/chobijaeyu/todoListAngular backend:https://github.com/chobijaeyu/todoListGin

service:

export class TodoService extends EntityCollectionServiceBase<Todo> {

  constructor(serviceElementsFactory: EntityCollectionServiceElementsFactory) {
    super("Todo", serviceElementsFactory);
  }

}

component.ts:

 deleteTodo(e: Event, todo: Todo) {
    e.stopPropagation()
    this.todoservice.delete(todo) 
// I tried this.todoservice.delete(todo.id)  same error
  }

error:

data.js:8106 TypeError: Cannot assign to read only property 'changeType' of object '[object Object]'
    at data.js:5990
    at Array.reduce (<anonymous>)
    at EntityChangeTrackerBase.trackDeleteMany (data.js:5968)
    at EntityChangeTrackerBase.trackDeleteOne (data.js:6023)
    at EntityCollectionReducerMethods.saveDeleteOne (data.js:6711)
    at entityCollectionReducer (data.js:7553)
    at EntityCacheReducerFactory.applyCollectionReducer (data.js:8025)
    at EntityCacheReducerFactory.entityCacheReducer (data.js:7749)
    at store.js:418
    at combination (store.js:306)

or

data.js:8106 TypeError: Cannot add property skip, object is not extensible
    at EntityCollectionReducerMethods.saveDeleteOne (data.js:6707)
    at entityCollectionReducer (data.js:7553)
    at EntityCacheReducerFactory.applyCollectionReducer (data.js:8025)
    at EntityCacheReducerFactory.entityCacheReducer (data.js:7749)
    at store.js:435
    at combination (store.js:323)
    at store.js:1282
    at store.js:403
    at computeNextEntry (store-devtools.js:999)
    at recomputeStates (store-devtools.js:1052)

Expected behavior:

what i expecte is delete item remote and local store.
actually it's did delete item remote,delete request return 200 even got this TypeError.

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

Angular CLI: 9.1.7
Node: 14.3.0
OS: darwin x64

Angular: 9.1.9
... animations, common, compiler, compiler-cli, core, forms
... language-service, localize, platform-browser
... platform-browser-dynamic, router, service-worker
Ivy Workspace: Yes

Package                           Version
-----------------------------------------------------------
@angular-devkit/architect         0.901.7
@angular-devkit/build-angular     0.901.7
@angular-devkit/build-optimizer   0.901.7
@angular-devkit/build-webpack     0.901.7
@angular-devkit/core              9.1.7
@angular-devkit/schematics        9.1.7
@angular/cdk                      9.2.4
@angular/cli                      9.1.7
@angular/flex-layout              9.0.0-beta.29
@angular/material                 9.2.4
@ngtools/webpack                  9.1.7
@schematics/angular               9.1.7
@schematics/update                0.901.7
rxjs                              6.5.5
typescript                        3.8.3
webpack                           4.42.0

    "@ngrx/data": "^9.2.0",
    "@ngrx/effects": "^9.2.0",
    "@ngrx/entity": "^9.2.0",
    "@ngrx/router-store": "^9.2.0",
    "@ngrx/schematics": "^9.2.0",
    "@ngrx/store": "^9.2.0",
    "@ngrx/store-devtools": "^9.2.0",

donated $20,thanks for your brilliant works.

[ionutkosteea]

Are there any news in regards to this issue. Will it be fixed soon?

[MHzarini]

i have same issue here, any updates?

[rexebin]

i have same issue here, any updates?

ditto..

This only happens when I specifically use addOneToCache(entity) after add entity.

I have this signalR service getting notifications on data changes on push, it process the notification and update cache if required. When a child entity is being created together with its parent, I need to manually add the child to its cache so that all client get updated.

I also find a very wired bug, if I subscribe to getByKey() and the returned entity in the stream of Observable is an old cached one instead of the one returned from remote! If I call the api via httpclient the new one returns correctly, but then I need to manually handle updateOneInCache and addOneToCache which leads to above delete problem.

[rexebin]

I went through the code base and found that in https://github.com/ngrx/platform/blob/master/modules/data/src/dispatchers/entity-dispatcher-base.ts, the actions ends with shareReplay(1). I had an exchange with @alex-okrushko and we agreed that shareReplay(1) is for sharing something for the lifespan of the entire application. With shareReplay(1), it keeps live even there is no subscribers.

Therefore I would suggest use shareReplay({buffSize:1, refCount:true}) in places of shareReplay(1). @brandonroberts

[brandonroberts]

That sounds reasonable to me. If you could add a test to verify the fix that would be great

[brandonroberts]

Someone still want to put in a PR for this fix?

[rexebin]

Not sure how to write a test for this, haven’t learned the dark magic behind shareReplay yet. I manually put a finally operator in the pipe and I could see shareReplay does not get destroyed when all subscriptions were unsubscribed. Definitely need to be very careful using this operator. Always set refcount to true, unless it is intended to stay throughout the lifetime of the app.

[rexebin]

The thing is, if adding refcount to these shareReplay operators in the code base doesn’t break current tests, I would vote for it.

[RepentAndBelieveTheGospel]

I've encountered this same issue, as described by @rexebin.

If I use addOneToCache after add, I get this:
TypeError: Cannot add property skip, object is not extensible
at EntityCollectionReducerMethods.saveDeleteOne (entity-collection-reducer-methods.ts:493)

If I use upsertOneInCache after add, I get this:
TypeError: Cannot assign to read only property 'changeType' of object '[object Object]'
at entity-change-tracker-base.ts:457

I've also noticed that if I use add then delete, it works. If I call add more than once consecutively (like in a file upload), then use delete, it crashes as shown above.

[AlmaniaM]

Is anyone working on this? If not, mind if I take it?

[brandonroberts]

@AlmaniaM go for it

[donohoea]

There is a lot here...but I think for the original issue of delete errors after using addOneToCache or upsertOneInCache after an add a quick fix would be to add noChangeTracking: true to your entity metadata to disable change tracking. Longer term it looks like there are two separate bugs related to deleting a record with change tacking enabled. The first in entity-change-tracker-base.ts the trackDeleteMany() method clones the chgState and then tries to modify the original Immutable chgState:

const trackedChange = chgState[id];        
if (trackedChange) {          
  //removed for brevity       
  } else if (trackedChange.changeType === ChangeType.Updated) {         
    cloneChgStateOnce();            
    trackedChange.changeType = ChangeType.Deleted;
 }...

Should actually be:

const trackedChange = chgState[id];        
if (trackedChange) {          
  //removed for brevity       
  } else if (trackedChange.changeType === ChangeType.Updated) {         
    cloneChgStateOnce();            
    chgState[id].changeType = ChangeType.Deleted;
 }...

The second bug is in entity-collection-reducer-methods.ts the saveDeleteOne() method tries to mutate the action payload inside the reducer. ngrx/data's EntityAction extends ngrx/store's Action which I believe are immutable? From entity-action.ts:

  // Mutable actions are BAD.
  // Unfortunately, these mutations are the only way to stop @ngrx/effects
  // from processing these actions.

  /**
   * The action was determined (usually by a reducer) to be in error.
   * Downstream effects should not process but rather treat it as an error.
   */
  error?: Error;

  /**
   * Downstream effects should skip processing this action but should return
   * an innocuous Observable<Action> of success.
   */
  skip?: boolean;

I'm not sure what to do with this, ngrx/data seems to require mutable actions.

@rexebin adding noChangeTracking: true to your entitymetadata should solve your issue with getByKey(), or if you require change tracking to be enabled you probably need to add { mergeStrategy: MergeStrategy.IgnoreChanges } option to your signalR service that is updating the cache based on push notifications.

The shareReplay issue seems like a completely separate issue but if its not getting destroyed it could be causing memory leaks. It is used most dispatcher methods that return and observable.

[RepentAndBelieveTheGospel]

@donohoea I can confirm that {mergeStrategy: MergeStrategy.IgnoreChanges} works.

this.someService.upsertOneInCache(
                    data.someEntity,
                    {mergeStrategy: MergeStrategy.IgnoreChanges}
                )

I've used it everywhere I do a cache action.

[donohoea]

@RepentAndBelieveTheGospel glad that worked for you, just curious are you using Change Tracking?

[AlmaniaM]

If anyone wants to make a PR for this go ahead. I've recently had something come up that's taking my attention away from this issue. I'll get back to this when I'm done and if no has fixed this yet.

[RepentAndBelieveTheGospel]

@donohoea Yes.

[donohoea]

I'd be willing to create the PR but would need a little advice on testing.

After some more thought on the issue I believe both errors are the result of store runtime checks and aren't thrown in production. The "Cannot assign to read only property 'changeType' of object '[object Object]'" error is coming from the strictStateImmutability check and as shown in a previous comment I think a simple change would fix the issue. Just not sure how to write a test that captures the failing runtime check.

As for the "TypeError: Cannot add property skip, object is not extensible," it's coming from the strictActionImmutability check. This could be solved by changing:

function ignoreNgrxAction(action: Action) {  
return action.type.startsWith('@ngrx');
}

to:

function ignoreNgrxAction(action: Action) {  
return action.type.includes('@ngrx');
}

I think this would capture ngrx/data actions in the exemption from strictActionImmutability as data actions are of the form: '[tag] @ngrx/data...' But again I'm unsure of how to capture the existing issue in a failing test. @timdeschryver maybe you have some thoughts as I believe you created the ignoreNgrxAtion function.