search

ianstormtaylor / superstruct

A simple and composable way to validate data in JavaScript (and TypeScript).
https://docs.superstructjs.org
MIT License
6.95k stars 224 forks source link

Defaulted should clone default value if it's an object #1248

Closed yeoffrey closed 1 month ago

yeoffrey commented 2 months ago

When using defaulted with a record, it should copy the default value if its an object. This will fix pass-by-reference bugs with this function.

Closes #1238

arturmuller commented 1 month ago

Thanks @yeoffrey for the PR — #1238 was resolved by merging in #1151 which adds coercers to record (and tuple), so I am closing this.

Having had some time to think about it, there are are two things that I realised:

  1. The custom class use-case is pretty much irrelevant. Superstruct operates primarily structurally, just like TypeScript. We have {...value} and array.slice() calls all over the place, so if you're using custom classes you're gonna have a bad time no matter what. This should be added to README somewhere.
  2. I think that structs should not touch data that is outside than their responsibility. This means that structuredClone is probably not something we want to ever use, since that makes a deep copy which is outside of its responsibility. I think the way superstruct is architected relies on every struct only every making shallow copies and leaving nested logic to subsequent/nested structs. I think this is a big strength, especially when masking and/or using the type struct, as it means that if there are parts of the source object that are not defined as a struct, they can be entirely skipped during processing. If the source objects are large, this can end up being quite efficient.