-
Notifications
You must be signed in to change notification settings - Fork 166
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
feedback on Refinement API #821
Comments
@ehaynes99 Hi, thanks for the feedback and again apologies for the delay in reply (have had some downtime recently and been trying to catch up on these issues)
Yeah, I largely agree here that it should be possible to narrow down from a wider type (say I had a quick poke around on TS 5.5 (nightly) and was able to get chained predicate narrowing working "sort of", but the inference isn't quite there yet (and I'm not sure if TS will provide support for more complex narrowing expressions as per the property type checks) import { Type, Static, TSchema } from '@sinclair/typebox'
// ----------------------------------------------------------------------------
// TRefined<T>
// ----------------------------------------------------------------------------
export interface TRefined<T> extends TSchema {
static: T
}
declare class RefineBuilder<T extends TSchema, S = T['static']> {
constructor(schema: T)
Check<U extends S>(refine: (value: S) => value is U): RefineBuilder<T, U>
Done(): TRefined<S>
}
declare function Refine<T extends TSchema>(schema: T): RefineBuilder<T>
// ----------------------------------------------------------------------------
// Usage
// ----------------------------------------------------------------------------
type A = Static<typeof A>
const A = Refine(Type.Unknown()).Check(value => typeof value === 'number').Done()
type B = Static<typeof B>
const B = Refine(Type.Unknown())
.Check(value => typeof value === 'object')
.Check(value => value !== null)
.Check(value => 'x' in value)
.Check(value => 'y' in value)
.Check(value => 'z' in value)
// .Check(value => typeof value.x === 'number') // these don't work :(
// .Check(value => typeof value.y === 'number')
// .Check(value => typeof value.z === 'number')
.Done()
Will keep an eye on things rolling through TS side with a focus on being able to express chained predicates (specifically sub property checks + binary expression checks)
The current implementation of Refine does support error messages in the following way (where you can pass a string that gets generated through failing checks). However, I don't know if I'm entirely happy with this design as I tend to think it's better to be less dependent on textual descriptions of errors and more centered on error codes (this specifically for internationalization of error messages), it's just that I haven't worked out a good approach for this. const T = Type.Object({
x: Type.Number(),
y: Type.Number()
})
const S = Type.Refine(T)
.Check(value => value.x === value.y, 'x must equal y')
.Done()
const R = Value.Check(S, { x: 0, y: 1 }) // const R = false
const E = [...Value.Errors(S, { x: 0, y: 1 })] // const E = [{
// message: 'x and y must be equal',
// value: { x: 0, y: 1 },
// ...
// }] Let's keep this discussion open for a bit. Would be good to have the open dialog here and discuss and document some of the rationales behind the Refine type (and how the type itself might become a pivotal type in future iterations of TypeBox). With the combination of import { Type, TSchema, RefineKind, Refinement } from '@sinclair/typebox'
const UnsafeByte = Type.Unsafe<number>({ type: 'byte' })
const Byte = Type.Refine(UnsafeByte)
.Check(value => typeof value === 'number')
.Check(value => !isNaN(value))
.Check(value => value >= 0)
.Check(value => value < 256)
.Done()
// Check without Value
function Check(schema: TSchema, value: unknown): boolean {
const refinements: Refinement[] = (schema as any)[RefineKind] ?? []
return refinements.every(refinement => refinement.check(value))
} Open to thoughts! |
Oh, right, I just meant that my pipeline-style example did not leave a place to set the error message. Interesting about that inference PR, but that's a syntactical convenience. With that example, you need to do both narrowing and widening, but it's still possible, just verbose: // inferred type:
// type B = {
// x: number
// y: number
// z: number
// }
type B = Static<typeof B>
const B = Refine(Type.Unknown())
.Check((value): value is object => typeof value === 'object')
.Check((value): value is object => value !== null)
.Check((value): value is { x: unknown } => 'x' in value)
.Check((value): value is { x: unknown; y: unknown } => 'y' in value)
.Check((value): value is { x: unknown; y: unknown; z: unknown } => 'z' in value)
.Check((value): value is { x: number; y: unknown; z: unknown } => typeof value.x === 'number')
.Check((value): value is { x: number; y: number; z: unknown } => typeof value.y === 'number')
.Check((value): value is { x: number; y: number; z: number } => typeof value.z === 'number')
.Done() It's worth noting that by the last step, we have explicitly defined the We can, of course, be more efficient, though. Even with the inference change in TS, I'm not sure which interpretation of type B = {
x: number
y: number
z: number
}
const B = Refine(Type.Unknown())
.Check((value): value is NonNullable<unknown> => value != null)
.Check((value): value is Record<string, unknown> => typeof value === 'object')
.Check(
(value): value is B =>
typeof value.x === 'number' &&
typeof value.y === 'number' &&
typeof value.z === 'number',
)
.Done() |
I was surprised to see the way typebox does this with the Is there anything we can do to help this effort? Refinements would make typebox the #1 validation library IMO. Edit: |
Sorry for the delay in response on this issue. I have been giving this feature a LOT of thought and unfortunately, I don't think I'm going to go ahead with it at this time. I'll explain my reasoning in a subsequent comment, but for now, I can suggest creating the Refine type by way of the TypeRegistry. TypeRegistry: Refine + ErrorsThe following implements Refine via the TypeRegistry as well as sets up custom Error messages. import { SetErrorFunction, DefaultErrorFunction } from '@sinclair/typebox/errors'
import { Static, Kind, TSchema, SchemaOptions, Type, TypeRegistry } from '@sinclair/typebox'
import { Value } from '@sinclair/typebox/value'
// ------------------------------------------------------------------
// Errors
// ------------------------------------------------------------------
SetErrorFunction(param => `errorMessage` in param.schema ? param.schema.errorMessage : DefaultErrorFunction(param))
// ------------------------------------------------------------------
// Refine
// ------------------------------------------------------------------
TypeRegistry.Set<TRefine>('Refine', (schema, value) => schema.func(value))
export interface TRefine<T = unknown> extends TSchema {
[Kind]: 'Refine'
static: T,
func: (value: unknown) => boolean
}
export function Refine<T>(func: (value: T) => boolean, options: SchemaOptions = {}): TRefine<T> {
return { ...options, [Kind]: 'Refine', func } as never
}
// ------------------------------------------------------------------
// Example
// ------------------------------------------------------------------
type T = Static<typeof T>
const T = Type.Object({
x: Refine<number>(value => typeof value === 'number'),
y: Refine<string>(value => typeof value === 'string'),
z: Refine<boolean>(value => typeof value === 'boolean', { errorMessage: 'Expected boolean' })
})
const V = { x: 1, y: 'hello', z: 'not a boolean' }
console.log(Value.Check(T, V)) // false
console.log(Value.Errors(T, V).First()) // 'Expected boolean'
Note that you can pass parameters by embedding them in the schema. The Refine function above embeds the Will post a follow up response as to why I'm going to hold off on this feature shortly |
Hesitation to include RefineSo, have given this feature a lot of thought, unfortunately, I'm not keen to pursue it at this time. My main reservation is that I don't like the idea of adding validation functions to schematics as functions are non-serializable (and thus non-portable). A core premise of TypeBox is that it should only construct schematic / metadata, with that metadata interpreted (or compiled) by something that understands it (i.e. a Json Schema validator). The problem with functions is that you cannot pass them to a remote system and expect that system to apply the same validation rules. I feel the inclusion of Refine breaks this core premise and doesn't address issues as to why devs feel the need to use Refine in the first place (ill talk about this a bit later) Consider the Byte type mentioned earlier (with and without Refine). Note that while Refine can be used to check the value, using TypeBox's built in types achieves the same result while benefitting from serializable / reflectable schematics. // You could do this....
const Byte = Type.Refine(UnsafeByte)
.Check(value => typeof value === 'number')
.Check(value => !isNaN(value))
.Check(value => value >= 0)
.Check(value => value < 256)
.Done()
// .. But why not?
const Byte = Type.Intersect([
Type.Number(),
Type.Number({ minimum: 0 })
Type.Number({ maximum: 256 })
])
// ... Or just?
const Byte = Type.Number({ minimum: 0, maximum: 256 }) Some HistoryAs mentioned, TypeBox was originally built on the premise that schematics "should" encode all the information required to validate a value, and that the schematics should be serializable and reflectable such that a remote system (i.e. C#) could read those schematics and generate the same assertion logic. Again, the problem with Refine is that JavaScript functions do not serialize, and because of this, they cannot be shared with remote systems needing the same validation rules.... With this said, I do appreciate that Refine is very ergonomic and able to reach places the TypeBox type system can't (forcing usage of the TypeRegistry), but feel it would be better to address (or at least explore) underlying inadequacies in the current schematics that require users to fall back to writing non-serializable functions. JavaScript Schema (Superset of Json Schema)Rather than adding Refine (which can be implemented by way of the current TypeRegistry). I am instead quite keen to explore additional extended schematics that enable TypeBox to encode more sophisticated validation rules. Unfortunately, I don't have all the answers as to what such a extended schematics should look like (maybe TypeBox should just embed ESTree AST??), but at a minimum, defining a superset of Json Schema that would enable validators to encode arbitrary assertion logic to check any JavaScript value would be the goal. Perhaps something along the lines of the following (as one possible example) // We want to validate this Class + method return value
class Add {
constructor(private a: number, private b: number) {}
evaluate(): number { return a + b }
}
// Perhaps this could work ??
const T = Type.ConstructorCall([1, 2], Type.Object({ // const T = {
evaluate: Type.FunctionCall([], Type.Literal(3)) // constructorCall: {
})) // parameters: [1, 2],
// returns: {
// type: 'object',
// required: ['evaluate'],
// properties: {
// evaluate: {
// functionCall: {
// parameters: [],
// returns: {
// const: 3
// }
// }
// }
// }
// }
// }
Value.Check(T, Add) // const instance = new value(1, 2)
// const result = typeof instance['evaluate'] === 'function' &&
// instance['evaluate'].apply(instance, [1, 2]) === 3 So, I haven't ruled out Refine as a future type, it's just not something I'm prepared to take on at this time. I think Refine would be very useful to perform auxiliary checks on values where the check logic is dependent on outside things (like checking a value is an instance of a MongoID), it's just that I'd prefer to seek out ways to encode such checks in the schematic metadata rather than turn to functions to perform the check. With all this said, I think identifying cases where users need to resort to TypeRegistry, then exploring ways of expressing that same checking logic as an encodable schematic ... this will be a good step towards moving TypeBox in the directions it needs to go. Hope this brings some deeper insights into the decision not to include Refine, will leave this issue open for comment. Cheers |
Thank you very much the in-depth explanation @sinclairzx81. I'm looking at my code now and one of the places I'm using Refine can be replaced with a better type. The other check could be moved out of the schema. Ultimately it makes a lot of sense what you're saying. It' a great goal to keep the schema as portable as possible. Out of curiosity have you thought about Refine seems to be valuable when it is not being used to enforce basic type constraints that would be otherwise possible to express in JSON Schema. Until it is revisited, the |
@jeremyjacob Hi,
Yes, have considered this. There was some apprehension about embedding Transform codec functions on the TB schematics also, however they were included on the basis that codecs do not (or at least should not) change the way values get validated. The schema is still the source of truth; and where the codec can be viewed as additional "application level processing" performed on a value before and after validation. Rx: [Input] -> [Validate] -> [Decode] -> [Output]
Tx: [Input] -> [Encode] -> [Validate] -> [Output]
// ^
// |
// these phases are swapped on encode and decode and where
// the validation is treated as a distinct phase from the
// encode/decode phase.
TypeBox interprets Decode/Encode errors as application level errors (separate from validation errors). In the case of Decode, if a value passes validation, the Decode function should ALWAYS succeed (as the value has been checked prior). Similarly, if a Encode function transforms a value, the resulting value should match the expected schematic. In both cases, failure to transform should be interpreted as an application level exception (a bug basically) In saying this, I have noticed that in the wild, a few users do attempt to use Transforms as a kind of secondary validation (where errors get thrown within the Decode and Encode functions on check failure). While this is of course possible, it's very much against the design intent for the Transform feature. I have actually been considering changing Encode and Decode to remove the implicit Check and having both functions return const T = Type.Transform(Type.Number())
.Decode(value => value.toString())
.Encode(value => parseFloat(value))
// ... possible future change (implicates transforming parse pipelines)
export function Parse<T extends TSchema, R = StaticDecode<T>>(schema: T, value: unknown): R {
const cleaned = Value.Clean(T, value) // cleaned is unknown
const converted = Value.Convert(T, cleaned) // converted is unknown
const defaulted = Value.Default(T, converted) // defaulted is unknown
// The change would require a user explicitly call Check() the value prior to calling Decode(). The
// Decode function may be updated such that the 'value' parameter must match a Static<T>. The return
// value would be updated to 'unknown' as it is not possible to statically assert a StaticDecode<T>
// due to external runtime processing of the value.
if(Value.Check(T, defaulted)) {
const decoded = Value.Decode(T, defaulted) // decoded is unknown
return decoded as R // Just trust decoded is R (unsafe)
} else {
throw Value.Errors(T, defaulted).First() // validation error
}
} I don't anticipate this change happening for a while. but do feel this change would draw a greater distinction between the Validation and Transform phases (as well as provide a bit more control for framework integrators) Hope this brings some insights! |
Hi all, Will close off this issue for now as there's not likely to be any changes here with respect to Refine (at least for now). I would recommend users interested in this feature experiment with implementations using the TypeRegistry (and I'm happy to include as a prototype if someone wants to submit a reference implementation). But moving forward, will be looking for ways to enhance the schematics beyond Json Schema to allow TB to both express and validate ALL JS constructs (and reduce cases where users need to opt for TypeRegistry) Thanks everyone! |
You asked for feedback here, but this is too much, so staring a new issue.
I definitely like the idea, but I'm not sure that it quite captures what such an API needs. Starting with the example:
This compiles into a check function like:
Each of the refinement steps has the type of:
This is fundamentally not correct. This means that the first check has the signature:
The actual type of
value
here isunknown
. The type system doesn't force users to deal with the initial conversion at all. Consider this simpler example:Now we could solve this by starting with
Type.String()
, but regardless, I think this misses potential.In functional programming, the term "refinement" has a common meaning of "function that narrows". Consider the signature:
Such a function either returns
false
ifT
is not aU
, or true, which narrows the type.U
defaults toT
, so we can have a check that doesn't narrow the TS type, but we're able to if we want, and I posit that we HAVE to at least once to transform theunknown
to the type we want. However, I would also say that we WANT to, because it would allow narrowing to branded types. This would allow TypeBox to express constraints not possible within the TypeScript type system.Branded types can be implemented by simply declaring a property of a symbol type not exposed to the outside world, thus ensuring that we've validated the type to end up with an instance.
Personally, I prefer a functional style. Unfortunately, TypeScript doesn't have a recursive definition with "type shifts" in the interim, so we have to use the "accordion hell" pattern. Still, it's in a library and the consumers don't have to care about it, so it's a bit of tedium once and then it's over (I find it easier to define the widest one first, then copy and delete one. It's a lot less error prone than hand-rolling each expansion).
This doesn't handle the error messages, though. We could make each step a type with both the function and the error message, but it could also use your function chaining style with a bit of work. I can spend some more time on putting together a PR, but not until next week (I know I promised you the fastify update a while ago too, but I'm really under the gun to convert a huge part of our system to TypeBox... hopefully there by Friday).
Playground link
The text was updated successfully, but these errors were encountered: