Closed TheDSCPL closed 7 months ago
Awesome ! Thank you very much @TheDSCPL ! Will have a look !
I'm not even sure DeepWritable
is needed anymore. Maybe I can handle readonly
types only as I think writable types extend them.
@TheDSCPL Should be fixed with https://github.com/ThomasAribart/json-schema-to-ts/pull/176
This fixes a bug that makes this library completely unusable (Typescript 5.1.6).
I got the
unknown
type when I tried the library on my straightforward schema. So, I tried to copy and paste the examples into my projects, but they didn't work either. After playing around with the library's types innode_modules
, I narrowed down the issue to the last ternary operator in the chain for the default value ofRESULT
in theParseSchema
type parameters, i.e.,SCHEMA extends SingleTypeSchema
. I tried changing it toSCHEMA extends {type: "object" }
, which also didn't work, which was strange, as my object had to at least match that.So I tried it directly in my source code:
This led me to believe the
Writeable
type wasn't working well, or at least it was too complex that my IDE was bailing early from the type calculation. I noticed there was no short-circuit for simple primitive types, so I prepended the type withTYPE extends number | string | boolean | bigint | symbol | undefined | null | Function ? TYPE :
andFoo1
was finallytrue
in my previous example (when I hovered my mouse over the type variable).So I tried the example again, but now
Dog
wasnever
.I then analysed the
FromSchema
type and copied it to my file to play around. I defined these types:And
Dog2
got the gigantic type definition:So I defined a more concise type to use as a replacement for
FromSchema
:and
Dog3
now had the same big type asDog2
. Meaning, the WRITEABLE_SCHEMA was breaking the types and that changing it to a simpler implementation (now thatDeepWritable
can handle simple types we don't need that type variable with a default value) made it resolve again, even if it was to a huge type and impossible to debug without letting TS narrow it down more.HOWEVER!
This, then, made me think that maybe my IDE was not doing a full type compilation when I hovered the mouse over to preview the type, so I went back to testing the original
DeepWritable
like this:My IDE said that type
FOO
wasfalse
, but there was a TS error on the assignment:TS2322: Type false is not assignable to type true
(!!!).In sum:
Clearly, this is a bug with my IDE's simplistic type resolution, but the type is also clearly overly complicated where it doesn't need to be, so there's no harm in simplifying the types, especially in types that drill down so much, and I decided to make this PR anyway.
My guess is that
eventually narrows down to the primitive types because of the prototype properties, which, later down, get duck-typed back to the primitive. Making the
DeepWritable
type simpler and faster is a win, IMO, which is why I changed it in this PR!Unless we want to allow primitive types with custom properties in JSON Schema, which are not even representable in JSON, there's no reason to break down primitives into their prototypes and then build them up again.
Note 1:
This is what I mean by primitives with custom properties:
Such an exotic type can't even be represented in JSON, so covering that case at the cost of build time makes no sense.
Note 2:
Allowing a
WRITABLE_SCHEMA
to be a type parameter makes no sense because its only type restriction is extendingWritableJSONSchema7
, so it would allow it to be used likeFromSchema<DogSchema, FromSchemaDefaultOptions, CatSchema>
, and it would basically be the same asFromSchema<CatSchema, FromSchemaDefaultOptions>
because the first type parameter is not actually used in the type's body, just in the 3rd type parameter's default value. So simplifyingFromSchema
also made sense to me!Note 3:
The tests passed and I also ran prettier and the linter.
P.S.:
Great job on this library and
ts-algebra
! They are really cool!