-
Notifications
You must be signed in to change notification settings - Fork 89
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
Enable Constraint to have a custom name and/or a custom static type #78
Enable Constraint to have a custom name and/or a custom static type #78
Conversation
Hi, thanks for the PR. I think rather than muddy the semantics of import { Runtype, create } from '../runtype';
import { ValidationError } from '../errors';
export interface Guard<V> extends Runtype<V> {
tag: 'guard';
name: string;
}
export function Guard<V>(name: string, guard: (x: any) => x is V) {
return create<Guard<V>>(
x => {
if (!guard(x)) throw new ValidationError(`Failed to pass type guard for ${name}`);
return x;
},
{ tag: 'guard', name },
);
} then you could use it like e.g. const RtBuffer = Guard('Buffer', Buffer.isBuffer); Note that this works because isBuffer(obj: any): obj is Buffer; For your own types you could make up arbitrary type guards, like const RtSomeClass = Guard(
'SomeClass',
(o): o is SomeClass => o._someClassTag === SOMECLASS_TAG
); This approach is more flexible than tacking something on to |
Hi @pelotom - I really like your idea of making a more generic solution as opposed to overloading only It'd also address the case where types and constraints are intertwined, e.g. Your idea might also also help with cases of non-public/undocumented properties or behavior, where the author deliberately wants to allow the runtime type to diverge from the TS type, e.g. to support legacy code using undocumented or deprecated features where the author doesn't want to expose them in a .d.ts but also doesn't want to fail vaildation if those undocumented things are used at runtime. I ran into this case recently with a MongoDB library's test code. That said, I've got a few concerns/questions. My main concern is that Seems like it'd be clearer to allow a single function to be used for both type-checking and custom-message-allowed constraint checking. Calling only one function also might yield a (minor) performance advantage for constraints called in repeatedly in loops or when type-checking huge arrays. Instead of (or in addition to?) adding a new top-level runtype, what do you think about a |
|
Any idea how that could work? TS type guard functions can't return strings and shouldn't throw if the type doesn't match. I guess What do you think? Is there a better way to do it? |
Hi @pelotom - Here's what I've got so far for adding custom error messaging to your If you like this direction I can PR it with tests, docs, reflection, etc. Let me know what you think. export function Guard<V>(
name: string,
guard: (x: any, errorReporter?: (message: string) => void) => x is V,
) {
return create<Guard<V>>(
x => {
let errMsg: string | undefined;
const errorReporter = (message: string) => {
if (String.guard(errMsg))
throw new Error('Cannot report more than one error from a type guard');
errMsg = message;
};
if (guard(x, errorReporter)) {
if (String.guard(errMsg))
throw new Error('Type guard must return false after reporting an error');
return x;
} else {
if (String.guard(errMsg)) throw new ValidationError(errMsg);
else throw new ValidationError(`Failed to pass type guard for ${name}`);
}
},
{ tag: 'guard', name },
);
} Here's some draft documentation with example usage: Custom type guardsIf an existing For example, many JavaScript classes use a static method for type checking because // const badDontUse = InstanceOf(Buffer); // breaks if different Buffer versions are used
const RtBuffer = Guard('Buffer', Buffer.isBuffer);
RtBuffer.check('not a buffer'); // Throws error: Failed to pass type guard for Buffer It's also easy to use a custom type guard function: const myClassGuard = (o: any): o is MyClass => o._myClassId === 'M Y C L A S S';
const RtMyClass = Guard('MyClass', myClassGuard);
RtMyClass.check('not a MyClass'); // Throws error: Failed to pass type guard for MyClass Validation messages can optionally be customized using type guards: const myClassGuard = (o: any, errorReporter?: (message: string) => void): o is MyClass => {
const id = o._myClassId;
if (!id) {
errorReporter && errorReporter(`MyClass id not found`);
return false;
} else if (id !== 'M Y C L A S S') {
errorReporter && errorReporter(`Invalid MyClass id: ${id}`);
return false;
}
return true;
}
const RtMyClass = Guard('MyClass', myClassGuard);
RtMyClass.check( {_myClassId: 'fake'} ); // Throws error: Invalid MyClass id: fake You can think of the |
After thinking about it some more, what do you think of this approach: export interface Guard<A extends Runtype, B extends Static<A>> extends Runtype<B> {
tag: 'guard';
underlying: A;
guard: (x: Static<A>) => x is B;
}
export function Guard<A extends Runtype, B extends Static<A>>(
underlying: A,
guard: (x: Static<A>) => x is B
) {
return create<Guard<A, B>>(
x => {
const typed = underlying.check(x);
if (!guard(typed)) throw new ValidationError(`Failed to pass type guard on ${show(underlying as any)}`);
return x as any;
},
{ tag: 'guard', underlying, guard },
);
} Some observations:
|
I really like this idea. Especially because it should make easier delegation of things like reflect/show. Speaking of One use case I was thinking about is where the implementer is OK with type-checking behavior of a runtype but wants more control over validation error messages, e.g. for localization or to provide clearer messages. How do you see this case working? Should the implementer select a lowest-common-denominator underlying type like
Yeah, I didn't like that callback API either. ;-) I just couldn't figure out any other way to make the guard functions used for I think a better approach might be to, instead of requiring a type predicate, just accept any function that returns boolean. Because the return type won't be a type guard, then devs won't expect them to act like type guards, so it's OK to throw exceptions for error messages. It'd be trivial for the Conveniently, a function that returns Another option I thought of was to borrow the Anyway, how about something like this? export function Guard<A extends Runtype, B extends Static<A>>(
underlying: A,
validator: (x: Static<A>) => boolean
) {
const guard = ((x: Static<A>): x is B => validator(x))
return create<Guard<A, B>>(
x => {
const typed = underlying.check(x);
if (!guard(typed)) throw new ValidationError(`Failed to pass type guard on ${show(underlying as any)}`);
return x as any;
},
{ tag: 'guard', underlying, guard },
);
}
Yeah, naming is tough-- if we're not actually giving a real guard function, then calling it |
I would probably just do something similar to what's done for
So if I understand what you're proposing correctly, this would make Another possibility would be to decouple customizing error messages from these particular runtypes and instead allow customizing any runtype's error message: Number.Or(String).withCustomErrorMessage((x, defaultErrorMessage) => `${x} is neither a number or a string`); Then a const BufferRuntype = Unknown
.withGuard(Buffer.isBuffer)
.withCustomErrorMessage(x => `${x} ain't a buffer yo!`); Something like
|
Hi Tom - sorry for slow reply, I got buried last week and just digging out now.
I think that would work too. I'll try to prototype it and work with it for a few days and let you know how it goes. One question: if Constraint could cast to a new type, then what would be the difference between Guard and Constraint? Would Guard be a subset of functionality as Constraint, but with an easier dev experience if you already have an existing guard function?
If we go with that extra type parameter to Constraint, then should we show the underlying type, the desired type, or both? I'm inclined to think that the desired type is the one to show to users, because the whole point of casting is to show a new face to the world. BTW, I have an embarrassing admission: I have no idea how the show/reflect parts of the library work. Is there an easy way to explain the parts about the recursive use of Reflect?
I definitely like the idea of being able to customize messages of existing runtypes, and your |
No worries, it often takes me a while to respond to PRs for the same reason.
Yes, I think if go ahead with augmenting
This is an interesting point; if you're changing the type then you probably want to show that somehow, so maybe we need a const PositiveNumber = Number.withConstraint(x => x > 0 | `${x} is not positive`, 'PositiveNumber');
The idea behind
I would think so. |
Hi @pelotom - finally getting back to this after kids' spring break and a weeklong adventure learning about how programmatically sending email that avoids Gmail spam folders is harder than it looks! I'm now trying to figure out what should be the signature for a I see that |
The That extra argument / type argument definitely complicates the types... for simplicity's sake let's imagine what it would look like if we didn't have to worry about export interface Constraint<A extends Runtype, T extends Static<A> = Static<A>> extends Runtype<T> {
tag: 'constraint';
underlying: A;
constraint(x: Static<A>): boolean | string;
}
export function Constraint<A extends Runtype, T extends Static<A>>(
underlying: A,
constraint: (x: Static<A>) => boolean | string,
): Constraint<A, T>;
// in Runtype:
withConstraint<T extends Static<this>>(
constraint: (x: Static<this>) => boolean | string,
): Constraint<this, T>; |
Given the existing 3rd-party dependency, would it be better to add a new export interface Constraint<A extends Runtype, K, T extends Static<A> = Static<A>>
extends Runtype<Static<A>> {
tag: 'constraint';
underlying: A;
// See: https://github.com/Microsoft/TypeScript/issues/19746 for why this isn't just
// `constraint: ConstraintCheck<A>`
constraint(x: Static<A>): boolean | string;
args?: K;
desired?: T;
name?: string;
}
export function Constraint<A extends Runtype, K, T extends Static<A> = Static<A>>(
underlying: A,
constraint: ConstraintCheck<A>,
args?: K,
name?: string,
): Constraint<A, K, T> {
return create<Constraint<A, K, T>>(
x => {
const typed = underlying.check(x);
const result = constraint(typed);
if (String.guard(result)) throw new ValidationError(result);
else if (!result) throw new ValidationError(`Failed ${name || 'constraint'} check`);
return (typed as unknown) as T;
},
{ tag: 'constraint', underlying, args, constraint, name },
);
}
// in Runtype:
function withConstraint<K>(constraint: ConstraintCheck<A>, args?: K): Constraint<A, K> {
return Constraint(A, constraint, args);
}
function withNamedConstraint<K>(
constraint: ConstraintCheck<A>,
name: string,
args?: K,
): Constraint<A, K> {
return Constraint(A, constraint, args, name);
}
function withRefinement<T extends Static<A>, K>(
constraint: (x: Static<A>) => boolean | string,
name?: string,
args?: K,
): Constraint<A, K, T> {
return Constraint(A, constraint, args, name);
}
function withGuard<T extends Static<A>, K>(
guard: (x: Static<A>) => x is T,
name?: string,
args?: K,
): Constraint<A, K, T> {
return Constraint(A, guard, args, name);
} What do you think of this kind of approach? If I'm on the right track, here's a bunch of open issues. Let me know if you have advice on these questions:
|
Yep, that makes sense to me.
I'm not sure I follow. Are you asking, should
I don't see a need for that, since we can recover
If we add the
It seems like
I think that's fine. The nature of this feature is about refining purely static types, so when static types get erased so does the distinction between a constraint and its base runtype.
I think so... I wonder if maybe the |
OK, here's what I've got so far with an In particular:
Runtype Declarations /**
* Use an arbitrary constraint function to validate a runtype, and optionally
* to change its name and/or its static type.
*
* @template T - Optionally override the static type of the resulting runtype
* @param {(x: Static<this>) => boolean | string} constraint - Custom function
* that returns `true` if the constraint is satisfied, `false` or a custom
* error message if not.
* @param {ConstraintOptions} [options]
* @param {string} [options.name] - allows setting the name of this
* constrained runtype, which is helpful in reflection or diagnostic
* use-cases.
*/
withConstraint<T extends Static<this>, K>(
constraint: ConstraintCheck<this>,
options?: ConstraintOptions<K>,
): Constraint<this, K, T>;
/**
* Helper function to convert an underlying Runtype into another static type
* via a type guard function. The static type of the runtype is inferred from
* the type of the guard function.
*
* @template T - Typically inferred from the return type of the type guard
* function, so usually not needed to specify manually.
* @param {(x: Static<this>) => x is T} guard - Type guard function (see
* https://www.typescriptlang.org/docs/handbook/advanced-types.html#user-defined-type-guards)
*
* @param {ConstraintOptions} [options]
* @param {string} [options.name] - allows setting the name of this
* constrained runtype, which is helpful in reflection or diagnostic
* use-cases.
*/
withGuard<T extends Static<this>, K>(
guard: (x: Static<this>) => x is T,
options?: ConstraintOptions<K>,
): Constraint<this, K, T>; Runtype Implementation function withConstraint<T extends Static<A>, K>(
constraint: ConstraintCheck<A>,
options: ConstraintOptions<K>,
): Constraint<A, K> {
return Constraint<A, K, T>(A, constraint, options);
}
function withGuard<T extends Static<A>, K>(
guard: (x: Static<A>) => x is T,
options: ConstraintOptions<K>,
): Constraint<A, K> {
return Constraint<A, K, T>(A, guard, options);
} Constraint Implementation export interface ConstraintOptions<K> {
name?: string;
args?: K;
}
export interface Constraint<A extends Runtype, K, T extends Static<A> = Static<A>>
extends Runtype<T> {
tag: 'constraint';
underlying: A;
// See: https://github.com/Microsoft/TypeScript/issues/19746 for why this isn't just
// `constraint: ConstraintCheck<A>`
constraint(x: Static<A>): boolean | string;
args?: K;
name?: string;
}
export function Constraint<A extends Runtype, K, T extends Static<A> = Static<A>>(
underlying: A,
constraint: ConstraintCheck<A>,
options: ConstraintOptions<K>,
): Constraint<A, K, T> {
return create<Constraint<A, K, T>>(
x => {
const typed = underlying.check(x);
const result = constraint(typed);
if (String.guard(result)) throw new ValidationError(result);
else if (!result) throw new ValidationError(`Failed ${name || 'constraint'} check`);
return (typed as unknown) as T;
},
{ tag: 'constraint', underlying, args: options.args, constraint, name: options.name },
);
} Reflection | {
tag: 'constraint';
underlying: Reflect;
constraint: ConstraintCheck<Runtype<never>>;
args?: any;
name?: string;
} & Runtype Show case 'constraint':
return refl.name || show(needsParens)(refl.underlying); |
Yep, that's what I had in my proposed types a few posts back.
I'm not sure I follow... why would anything change with regards to the runtime checks being done? If you want no runtime checks apart from those provided by the type guard, you would just use
Looks right to me.
I'm really the wrong person to ask about jsdoc syntax 😆
Yeah, I'd say so. |
386ccd2
to
18d8a1d
Compare
Hmm, didn't mean to close this... was resetting to master before pushing latest code. Hopefully will get re-opened once I push! |
Nope, it didn't re-open after I pushed new code. ;-( Perhaps it's a blessing in disguise given how far this PR has morphed since the beginning. I'll open a new PR and will link back to the discussion in this one. |
Reopened. |
41d1777
to
e79b081
Compare
OK, after a long and winding path to this PR, here's the latest. I think that all open issues from our last go-round have been resolved. Let me know what you think. I can also commit documentation changes too-- but didn't want to do this until we were sure that the API and behavior was going to stick. ;-) This PR adds two features to the
To set the name, use the new, optional const C = Number.withConstraint(n => n > 0, {name: 'PositiveNumber'}); To change the type, there are two ways to do it: passing a type guard function to a new Using a type guard function is the easiest option to change the static type, because TS will infer the desired type from the return type of the guard function. // use Buffer.isBuffer, which is typed as: isBuffer(obj: any): obj is Buffer;
const B = Unknown.withGuard(Buffer.isBuffer);
type T = Static<typeof B>; // T is Buffer However, if you want to return a custom error message from your constraint function, you can't do this with a type guard because these functions can only return boolean values. Instead, you can roll your own constraint function and use the const check = (o: any) => Buffer.isBuffer(o) || 'Dude, not a Buffer!';
const B = Unknown.withConstraint<Buffer>(check);
type T = Static<typeof B>; // T is Buffer One important choice when changing This new Constraint implementation is backwards-compatible to existing users, except for the small minority of users who were using the undocumented
Tip: don't reset your forked branch to master and force-push. GitHub will close your PR! Oops. |
Sorry for the delay in getting back to this. It looks really great! If you could add a section to the |
One other thing that occurred to me: it seems likely that export const Guard = <T, K = unknown>(
guard: (x: unknown) => x is T,
options?: { name?: string; args?: K },
) => Unknown.withGuard(guard, options); which would allow you to just write |
Great, sounds like we're in the home stretch. Yep, was planning to slightly adapt the text in my last post into README content once we finalized the API, which it sounds like we just did! Your BTW one interesting issue I ran into when using the new constraint type was that you can't use a function like |
Nope, it’s just an alias for a particular configuration of
This is an interesting point. function (x: unknown): x is { foo: boolean } {
return typeof x === 'object'
&& x !== null
&& 'foo' in x
&& typeof x.foo === 'boolean';
} This is all perfectly logical, but unfortunately the type system gets stuck on the last step, saying that Anyway, I agree that it's more convenient to define type guards taking export const Guard = <T, K = unknown>(
guard: (x: any) => x is T,
options?: { name?: string; args?: K },
) => Unknown.withGuard(guard, options); This seems like it's probably worth doing, even though it's a little bit less type-safe. What do you think? |
On the other hand, this would mean if you had a type guard which did make some assumptions about its input, e.g. |
This was exactly the thought process I went through. My best idea was to make the I did manage, while trying out various syntax options, to trigger some amazingly typescript perf issues where intellisense would hang for minutes at a time with my MacBook fan running at full blast, so I guess I did achieve something at least. ;-) |
I think I’m in favor of keeping it simple and leaving it as |
Yep agreed |
e79b081
to
a7ec3b8
Compare
a7ec3b8
to
2e4ec41
Compare
OK Tom, I think it's (finally) ready! Here's what different from last time:
Sorry it's taken so long to finish this, and thanks for your feedback... I learned a lot! Holler if you see any problems. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thanks for all your hard work!
Cool. Unfortunately I can't use it on TS 3.5 because of #89. Any idea how to fix that issue? |
Nope, haven't had time to look at it yet. |
BTW I figured out a fix for the TypeScript 3.5 problems and submitted a PR to fix them. |
EDIT: this PR changed a lot, and got more general, so this initial post is not where the solution is heading. See discussion below.
This PR adds an optional parameter
customInstanceOf?: (o: V) => boolean
to theInstanceOf
constructor. This parameter lets clients optionally use a custom function instead of the defaultinstanceof
keyword to decide if an object is an instance of the specified class.Justification
This is helpful in cases where multiple library versions might be running side-by-side in the same process. This is unfortunately common in a node.js environment, e.g. you depend on library A and library B which each depend on different versions of library C.
Usually libraries which want to run side-by-side will have a static
isXXX
method as a version-safe alternative toinstanceof
. Examples:Buffer.isBuffer
- from Node.js APIObjectId.isValid
- from MongoDB client API for Node.jsThese isXXX functions are usually implemented via checking a special property (e.g.
_bsontype
for some classes in the MongoDB client library) that's used for the purpose of identifying instances of the same class across versions. So even if a library doesn't offer an isXXX function, the client can easily build one and pass it to thecustomInstanceOf
parameter in this PR.Tests
I added tests for this PR that:
check()
to returntrue
when comparing instances of different versions. Existing tests with the defaultinstanceof
behavior still pass.InstanceOf
tests forDate
andRegExp
objects. This is unrelated to this PR but those two built-in classes are (in my experience) frequent sources of class-related validation bugs so it's good to have them tested!BTW, this library seems great. I'm in the middle of converting my app over to use it, and so far the only serious gap I've found is here in this PR. If you want me to change anything about this PR, I'm happy to do it-- just let me know. Thanks!