sinclairzx81 / typebox

Json Schema Type Builder with Static Type Resolution for TypeScript
Other
4.65k stars 150 forks source link

Unable to create value of custom class type #817

Closed seppelandrio closed 3 months ago

seppelandrio commented 4 months ago

Hello,

in my project I need to provide a schema for a custom class (ObjectId). Unfortunately my usage of Value.Create breaks when switching from v0.25.24 to latest version as I need to provide a default value but this one needs to be cloneable according to create.ts FromDefault what obviously fails:

import { Kind, TypeRegistry } from "@sinclair/typebox";
import { Assert } from "../../assert";
import { Value } from "@sinclair/typebox/value";

describe("value/create/UserDefinedType", () => {
  describe("For custom class", () => {
    it("Should create default", () => {
      class CustomClass {}

      TypeRegistry.Set(
        "CustomClass",
        (_, value) => value instanceof CustomClass
      );

      const T = {
        [Kind]: "CustomClass",
        default: new CustomClass(),
        static: new CustomClass(),
        params: [],
      };

      Assert.IsInstanceOf(Value.Create(T), CustomClass);
    });
  });
});

Would it be possible to extend the clone method to also support custom classes by just providing the value similar to clone.ts ValueType or is there an alternative fix?

Thanks in advance for your response!

seppelandrio commented 3 months ago

Any updates here?

sinclairzx81 commented 3 months ago

@seppelandrio Hi, Apologies for the delay (have been somewhat busy of late)

So, the issue here is that TypeBox clones schematic when you compose them. It does this to ensure manual modifications to types do not overwrite the properties of types elsewhere.

const A = Type.Number()

const T = Type.Object({
  a: A // cloned / shared schematic
})

A.foo = 'hello'
T.properties.a.foo = 'world'

console.log(A)                // { type: 'number', foo: 'hello', [Symbol(TypeBox.Kind)]: 'Number' }
console.log(T.properties.a)   // { type: 'number', foo: 'world', [Symbol(TypeBox.Kind)]: 'Number' }

So from this, when you specify a class instance for the default property, TypeBox will naively attempt to clone that class instance as an Object type. However when doing so, it will loose track of the instance type (so CustomClass) and yield a empty {}.


Unfortunately, I'm not sure there's much TypeBox can do here to ensure the class instance type is retained during composition clone. The best advise I can offer at this stage is to avoid using class instances for default values (or any schema property). Ideally, you should aim to ensure all properties passed to schematics are vanilla objects, arrays, strings, numbers, booleans, etc and avoid classes and functions being assigned. This is generally a good idea as it means your schematics can be serialized correctly (noting classes and functions do not serialize as Json).

It might be possible to implement a specialized clone for class instances (ill look into it), but I'm not sure it can be done safely without also cloning the class instance prototype. Ideally i'd like to have TypeBox's clone work a little closer to JavaScript's existing structuredClone function, but as TypeBox does implement some extras for Symbol handling, it's not out of the realms of possibility to include something to also handle class instance cloning, it's just that additional functionality for clone isn't anticipated in the short to medium term...

I think for now I will close off this issue for the time being as class instance cloning is somewhat out of scope (at least for now) and the recommendation is just to avoid using class instances on types. Would be happy to discuss things further on this thread though (or GH discussion thread). To get movement on adding class instance cloning, would need to see a robust implementation of it first. If you wanted to submit a code snippet of it, would be happy to review and experiment for possible inclusion at some point later down the road.

Hope this helps S

seppelandrio commented 1 month ago

@sinclairzx81 Hi,

this time sorry for the delay from my side, needed to focus on different projects. I agree that writing a class cloner will be somewhat complex and probably not worth the effort. What I had in mind was to provide either:

With those 2 approaches you could easily add custom types by just providing the functionality you need. About which approach to choose I am quite unsure as I am not that familiar with the codebase. If you think that one of those options seems worth a try I can also try to provide a PR implementing the approach.