lukeed / klona

A tiny (240B to 501B) and fast utility to "deep clone" Objects, Arrays, Dates, RegExps, and more!
MIT License
1.62k stars 43 forks source link

Clona fails to copy an object, when it contains a `DateTime` from Luxon #33

Closed ropez closed 3 years ago

ropez commented 3 years ago

Reproduction code (tested with both Luxon v1.25 and v2.0.2):

import { klona } from 'klona/lite';
import { DateTime } from 'luxon';

const data = {
  date: DateTime.fromISO('2021-10-01'),
};

const copy = klona(data);

Expected result: copy is a new object, caontaining the same date as the original. Since DateTime is immutable, it doesn't need to copy it.

Actual result: Exception:

luxon.js?1315:6204 Uncaught TypeError: Cannot read properties of undefined (reading 'zone')
    at new DateTime (luxon.js?1315:6204)
    at klona (index.mjs?ba1d:8)
    at klona (index.mjs?ba1d:25)
    at eval (main.ts?cd49:30)

Problem scope: This is a problem for us, when using vee-validate, with is using klona internally, and we are using dates in our custom form components. There's no way to opt out of this behavior, which is just crashing our page after upgrading vee-validate.

Suggestion: Obviously, it's not in the scope of this package to be compatible with everything that's out there. Although, it is questionable to call constructors of unknown classes, and then try to copy field by field. This will probably fail for a large number of classes. There should be some way to opt-out of the copying, and make klona copy by reference, which would be OK in this case. Maybe it would be possible to mark objects with a certain property, to make then "non-copyable" by klona. Even better, mark the constructor of the class in question, so that all objects of that class are copied by reference.

lukeed commented 3 years ago

A library like vee-validate should be using klona/full since it has no idea what the user(s) will be using and should be ready for any/all input types. To your point, this is the only mode that doesn't call the source's constructor and instead takes a more comprehensive approach.

I reran your repro (thanks for including) and can show that klona/full works as expected:

const { klona } = require('klona/full');
const { DateTime } = require('luxon');

const input = {
    date: DateTime.fromISO('2021-10-01')
}

let output = klona(input);
console.log({ output });
Screen Shot 2021-09-29 at 11 17 24 PM

Also, if vee-validate is unwilling to adjust their klona import, you can use a build-time resolution alias/override to map klona/lite to klona/full instead. All klona modes have the same API for this reason.

ropez commented 3 years ago

Thank you for the suggested work-around.

I looked briefly at the "full" implementation now. To be honest, I'm still skeptical to copying an object, in an object-oriented environment, value by value. It breaks basic principles of OO like encapsulation. Wouldn't it break easily, for instance if the DateTime class depended on some closure created in it's constructor? Future versions might break.