Platform: [entity] updateOne, updateAll and map methods on EntityStore clear entity prototypes

[mtaran-google]

Minimal reproduction of the bug/regression with instructions:

1. Put class instances as entities inside an ngrx/entity store
2. Try to use the above EntityAdapter operations on them
3. The operations work, but the objects turn into plain objects (prototype is just Object)

Expected behavior:

The prototype chain on the entities is preserved.

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

all (pretty sure)

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

Maybe, if nobody else is interested :o

Seems like you'd need to do a get/setPrototypeOf near https://github.com/ngrx/platform/blob/master/modules/entity/src/unsorted_state_adapter.ts#L100 & the sorted equivalent

[brandonroberts]

@mtaran-google this is intentional. More context is provided here https://github.com/ngrx/platform/issues/1376#issuecomment-431691078

[mtaran-google]

I was aware that it's a best practice to use plain objects with ngrx/redux, but also that most things generally work fine/as expected if you don't.

Is this constraint documented somewhere on https://ngrx.io/docs? If not, this bug can be refocused on just spelling out that this isn't a well-supported use case.

[brandonroberts]

It is documented here currently https://ngrx.io/guide/entity#entity-and-class-instances

We can add more if it's not explicit enough

[mtaran-google]

I see. That is detailed, though it's not actually accurate. You only run into problems when you use the three helpers listed -- otherwise the objects are kept as they are. In fact if other helpers started messing with our objects, that would be a significant breaking change and our project would have a hard time dealing with it, and I suspect the same is true for others. It would be nice if this was more explicit.

[alex-okrushko]

@mtaran-google Yes, we ran into this issue as well (due to the generated classes).
I think we could provide custom clone function (instead of Object.assign({}, original, update.changes) which looses getters and setters) that would solve the problem.

The current solution is to serialize the class before passing it to those entity methods.

[mtaran-google]

That would be nice! For now we're just avoiding those methods, which is not too terrible, but still not the best.

[alex-okrushko]

@mtaran-google I sent you the link with the serialization workaround - not sure if you saw it before it was deleted. Ping me if you missed it :)

[ankemp]

Hey @alex-okrushko would it be possible to send me that workaround as well?

[alex-okrushko]

@ankemp I had the internal specific solution.
The main idea is to turn/serialize the class into an object.

[ankemp]

@alex-okrushko thanks, I'll play around with that idea. For now I ended up modifying the entity adapter

[timdeschryver]

I'm going to close this issue because it's inactive and 'I think that this is solved.
@alex-okrushko feel free to re-open if that's not the case.

[godenji]

Voting to re-open -- would prefer that the entity type is preserved rather than sweeping the rug out from under the user and magically converting to object.

I suspect most users serialize state in IndexedDB, Sqlite, etc. as plain objects, and when pulling state from the datastore, convert to class type, interface, or whatever they want.

What's surprising is performing an operation like upsertOne and finding that selected entity is correctly of type T, but entities array now has upserted entity as plain JS object.

It would make sense if this constraint were applied across the board, or not at all. The current status quo of sometimes converting to object, sometimes not, leads to surprising bugs -- only after stepping through the debugger did I find the source of my WTF moment with upsertOne.

Worth noting the Redux' docs on the issue only suggest avoiding storing class instances in the store.

Why not give users the option of preserving class type?

[NedAnd1]

@alex-okrushko I also vote to re-open!
An optional parameter such as ctor: new () => T would allow Object.assign( ctor? new ctor() : {}, a, b ) to be written in the place of Object.assign({}, a, b). Behavior for current users wouldn't change, but developers could opt-in to entities retaining their expected type e.g. createEntityAdapter({ctor:BookClass})

pretty sure getters & setters are bound to an object's __proto__, but already the ngrx store's serializability & immutability can be violated in several other ways; for absolute immutability explicitly calling Object.freeze is the only way I know, but that would lead to similar inconsistencies unless the hammer's dropped & freeze was called for every input. We're already responsible for making sure our reducers maintain state, so why not allow us to be responsible for this as well?

[ankemp]

I did start working on a it a bit ago. https://github.com/ankemp/platform/commit/1082c2673e5b3eff12c61be2fec743508581dd41 I know the idea works (using Object.create(Object.getPrototypeOf(original))) but I'm not sure if the what I did the config is the right way to go about it.

[NedAnd1]

@ankemp I like what you did as well; I used the ctor thing for getting typed objects from the server w/ mapping annotations for cases where format differs.