Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed

Conversation

aleclarson
Copy link

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).

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
Copy link
Owner

@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 #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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants