@ngrx/data - mergeServerUpserts - can't define property "originalValue": Object is not extensible

[AdditionAddict]

Minimal reproduction of the bug/regression with instructions:

Stackblitz - see mergeServer()

Here after setup of initialState the state is

    const result = {
      entityCache: {
        Hero: {
          ids: [1, 2, 4],
          entities: {
            "1": { id: 1, name: "UnchangedEntity", power: 1 }, // unchanged
            "2": { id: 2, name: "AddedEntity", power: 2 }, // added
            "4": { id: 4, name: "UpdatedEntity", power: 40 } // updated
          },
          entityName: "Hero",
          filter: "",
          loaded: true,
          loading: false,
          changeState: {
            "2": { changeType: 1 },
            "3": {
              changeType: 2,
              originalValue: { id: 3, name: "DeletedEntity", power: 3 } // deleted
            },
            "4": {
              changeType: 3,
              originalValue: { id: 4, name: "UpdatedEntity", power: 4 } // updated
            }
          }
        }
      }
    };

Expected behavior:

Would expect state to be the same (merge values are the same) with no errors

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

ngrx 8.6.0

Other information:

mergeServerUpserts in https://github.com/ngrx/platform/blob/master/modules/data/src/reducers/entity-change-tracker-base.ts#L296

      case MergeStrategy.PreserveChanges: {
        const upsertEntities = [] as T[];
        changeState = entities.reduce((chgState, entity) => {
          const id = this.selectId(entity);
          const change = chgState[id];
          if (change) {
            if (!didMutate) {
              chgState = { ...chgState };
              didMutate = true;
            }
            change.originalValue = entity; <---------------------------------
          } else {
            upsertEntities.push(entity);
          }
          return chgState;
        }, collection.changeState);

I would be willing to submit a PR to fix this issue

[x ] Yes (Assistance is provided if you need help submitting a pull request)
[ ] No

[AdditionAddict]

Can't replicate on pure mergeServerUpserts unit tests. Starting to think this is related to the UPSERT_ONE in the set up of the initialCache
Same as #2307 and related to immutability? trackUpsertMany used in both cases

More minimal stackblitz

[AdditionAddict]

@timdeschryver can I borrow your knowledge on immutability and check my thinking on this before submitting a PR?

it concerns mergeServerUpserts https://github.com/ngrx/platform/blob/master/modules/data/src/reducers/entity-change-tracker-base.ts#L296

Is the reason this throws because in the case MergeStrategy.PreserveChanges the line const change = chgState[id]; is still referencing the original state rather than a copy and the line change.originalValue = entity; is trying to change the original state? So a fix would be to do the following?

const change = chgState[id]; // original state
if (change) {
    if (!didMutate) {
        chgState = {
            ...chgState,
            [id]: {
                ...chgState[id]!, // ! because of the `if (change)`
                originalValue: entity
            }
        };
        didMutate = true;
    }
} 

Should I include a tests along the following lines when submitting PR?

    describe('MERGE_QUERY_SET', () => {
      it('should change state in an immutable way', () => {
        let {
          addedHero,
          initialCache,
        } = createInitialCacheForMerges();
  
        const test = function() {
          'use strict';
          Object.freeze(
            initialCache.Hero.changeState[addedHero.id]!
          );

          const querySet: EntityCacheQuerySet = {
            Hero: [addedHero],
          };
          const action = new MergeQuerySet(querySet);
          const state = entityCacheReducer(initialCache, action);
    
          // check state does not hold copy of initialCache
          state['Hero'].changeState[addedHero.id]!.originalValue = null; 
        };
        expect(test).not.toThrow();
      });

[timdeschryver]

@AdditionAddict correct, because it has the same reference it will throw.

(function()
{
    'use strict';
    let person = { id: 1, name: 'Alice' }
    Object.freeze(person)
    let arr = [person]
    let p = arr[0]
    p.name = 'boom'
}());

I think your proposed fix should resolve this.