feathers-hooks-common icon indicating copy to clipboard operation
feathers-hooks-common copied to clipboard

stashBefore serializes output and flattens date objects

Open FossPrime opened this issue 7 years ago • 4 comments
trafficstars

https://github.com/feathers-plus/feathers-hooks-common/blob/5d860579da32be9da3bce0d55f5cbd135b2f50d2/lib/services/stash-before.js#L33

Why does stashBefore serialize the object? this destroys Date objects for apparently no reason, which is unlike what you would expect from something that does a get call and stashes the result.

This matters because it's a lossy conversion that's hard to undo... which you would need to do if we were to make a validateSchema version that works on Patch calls by piggy backing on the expensive stashBefore hook

FossPrime avatar Sep 21 '18 22:09 FossPrime

Its a cheap way of cloning the result. Adapters like feathers-nedb and feathers-memory pass back the actual object. It you stash that, the stashed results will mutate as the record mutates.

eddyystop avatar Sep 21 '18 22:09 eddyystop

stashBefore has issues as the stashed record may not be the actual DB record, but one mutated by the after hooks. I'll be modifying the hook if some recent information regarding getting raw records turns out to be what I hope.

eddyystop avatar Sep 21 '18 22:09 eddyystop

I get it's cheap, but it can end up being more expensive if we can't rely on it to accurately stash things for other hooks such as a would be patchFill or inheritPatch hook for patch, that takes all the data in the original object and merges it for running a strict validator on it. That is, if we end up having to use it and also make yet another get call on another hook.

or any number of potential future common hooks or like in my case, custom hooks. I'm not sure how much more expensive Object.assign would be... but I'm willing to pay the price. Not sure if there is a way to avoid cloning and still give out errors if we attempt to mutate it in a dev environment.

FossPrime avatar Sep 21 '18 22:09 FossPrime

I can add a second param you can use to pass a clone() function to stashBefore.

Clone functions are bulky. Hooks can be used client-side so the repo tries not to pull in dependencies which may not be used but which increase the bundle size.

eddyystop avatar Sep 22 '18 12:09 eddyystop