fix(data): immutably delete an entity #3040
Conversation
|
Preview docs changes for 6944383 at https://previews.ngrx.io/pr3040-69443838/ |
modules/store/src/runtime_checks.ts
Outdated
| @@ -69,7 +69,7 @@ export function createImmutabilityCheckMetaReducer({ | |||
| } | |||
|
|
|||
| function ignoreNgrxAction(action: Action) { | |||
| return action.type.startsWith('@ngrx'); | |||
| return action.type.includes('@ngrx'); | |||
There was a problem hiding this comment.
Why do we need this change? I don't think it should be included in this PR
There was a problem hiding this comment.
One of the errors in #2553 was a TypeError: Cannot add property skip, object is not extensible thrown by the strictActionImmutability runtime check. Since action mutation is baked into ngrx/data with use of the skip and error fields in EntityActionOptions it makes sense to exempt ngrx/data actions from the runtime check. As data action types take the form: '[tag || entityname] @ngrx/data.....' they aren't captured by startsWith('@ngrx').
There was a problem hiding this comment.
AFAIK the delete action is the only one that's causing this problem (I could be wrong).
Can't we make it immutable instead?
There was a problem hiding this comment.
Yes this issue is only related to the delete action. But the action is mutated by design. When change tracking is enabled (by default) on entity if an add or update is persisted to the local store but not to the server (ie. no add success action is dispatched) and a subsequent delete action is dispatched the reducer removes the entity from the local store and adds the skip flag to the action so the effects ignore it and do not try to delete a record that never got to the server.
In a similar manner I believe there are cases where other actions would cause an error in a reducer method which would add the error flag to the action so that the action is ignored by the effects. From the definition of the ngrx/data entity-action:
...
// 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 don't think we can make data actions immutable without some significant changes.
There was a problem hiding this comment.
I'd rather not change the behavior of the startsWith check, but change the action string for ngrx/data. It could be considered a breaking change is someone is listening based on that action type, but it's unlikely they are using the actual string itself.
There was a problem hiding this comment.
We could keep this in mind while working on v13 and leave it as it for now?
There was a problem hiding this comment.
Yea, i think that would be better
There was a problem hiding this comment.
Ok I can change the ngrx/data action string instead. There is a test fix and a fix for mutable state change bug also identified in #2553 in this PR as well. These would not introduce breaking changes. Should I spin them out into separate PRs so they can be merged before v13? Maybe they should have been separate from the beginning? New to the PR process.
There was a problem hiding this comment.
You can do them in PR as separate commits and we'll just keep the separate commits instead of squashing them into one
|
@donohoea will you rebase on |
|
@donohoea changing the action type format is a breaking change that will have to wait until v13. |
|
@brandonroberts that makes sense. Do you need me to remove that commit or what's the best way to hold on to that for v13? |
|
@donohoea yes, just remove the commit and we'll keep it on the list for v13. |
'NgRx' actions were passing tests by default as no case handled them in the reducer. Adding cases to handle 'NgRx' actions exposed a futher error in the tests. 'should not throw for NgRx actions' test was testing and failing against an Unserializable state. The library allows NgRx actions where the action is Unserializable so the 'should not throw for NgRx actions' test was moved from State Serialization to Action Serialization.
timdeschryver
changed the title
|
Thanks @donohoea |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Closes #2553
What is the new behavior?
No state mutation on delete and ngrx/data actions are exempt from strictActionImmutability runtime checks.
Does this PR introduce a breaking change?
Other information