sinclairzx81 / typebox

Json Schema Type Builder with Static Type Resolution for TypeScript
Other
4.77k stars 152 forks source link

fix: respect Optional in Transform.Encode/Decode #783

Closed aleclarson closed 6 months ago

aleclarson commented 6 months ago

When a Transform is optional and the value is undefined, let‘s avoid calling the Encode/Decode callbacks at all so they don‘t have to be responsible for how undefined values are handled (well, when Optional is used at least).

sinclairzx81 commented 6 months ago

@aleclarson Hi,

Thanks for this PR, but can't accept this one as this would be breaking in terms of how TypeBox reasons about Optional and Undefined. Ill explain a bit below.


Optional vs Undefined

As per comment on https://github.com/sinclairzx81/typebox/pull/782

Note that Optional type is only applicable to PropertKeys of Objects where optionality is determined via key in value checks (as opposed to value === undefined) so this logic might make more sense added to the FromObject method (where key enumeration happens). Also note that ability to bail out on Optional may be applicable to all types (so not limited to Union), which also suggests updates to FromObject.

Note that Optional doesn't necessarily mean undefined, rather it's a constraint more applicable to PropertyKeys. Like in TypeScript, TypeBox treats undefined as a unit type (similar to null) meaning it is treated as a kind of value type. By default however, TypeScript will implicitly assign a property to be T | undefined if the property is Optional...but this doesn't necessarily imply that the property value can be undefined...

The implicit assignment of T | undefined can be a bit misleading as it gives the impression that a property value of { value: undefined } is equivalent to { }. From a TypeScript perspective, this is somewhat ambiguous (but permissible for practicality reasons), from a JavaScript specific standpoint, this is loosely true.

const A = { value: undefined } // has 'value' key
const B = { }                  // has no 'value' key

const R1 = 'value' in A // true
const R2 = 'value' in B // false

A.value // undefined ?
B.value // undefined ?

Note that TypeScript does actually provide a configuration override to fix up implicit T | undefined with exactOptionalPropertyTypes. Additional information on this configuration can be found here https://www.typescriptlang.org/tsconfig#exactOptionalPropertyTypes

{
  "compilerOptions": {
     "exactOptionalPropertyTypes": true // Interpret optional property types as written, rather than adding undefined.
  }
}

TypeBox also supports this configuration https://github.com/sinclairzx81/typebox?tab=readme-ov-file#policies

import { TypeSystemPolicy } from '@sinclair/typebox/system'

// Disallow undefined values for optional properties (default is false)
//
// const A: { x?: number } = { x: undefined } - disallowed when enabled

TypeSystemPolicy.ExactOptionalPropertyTypes = true

Undefined Transforms

So, with the above mentioned, and in the context of Transforms specifically (with the knowledge that TB treats undefined as a value), the following Transform type should continue to be possible.

import { Value } from '@sinclair/typebox/value'
import { Type } from '@sinclair/typebox'

const T = Type.Transform(Type.Undefined())
  .Decode((value) => 'hello world')
  .Encode((value) => undefined)

// Reference Example

console.log(Value.Decode(T, undefined)) // "hello world"

Hope the above makes sense, there's a bit of detail around optional / undefined handling in TypeBox. But ultimately, TypeBox does try to follow the TypeScript view of JavaScript. So from this, any logic that interprets undefined meaning the same as optional would be considered invalid based on the semantics of TypeScript.

Will close up this PR as things should be working as expected here. Cheers!