-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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
feat(@hapi/joi): prevent incorrectly infering Joi.object type from user provided parameters #41173
feat(@hapi/joi): prevent incorrectly infering Joi.object type from user provided parameters #41173
Conversation
…er provided schema
@HosseinAgha Thank you for submitting this PR! 🔔 @Bartvds @laurence-myers @cglantschnig @DavidBR-SW @GaelMagnan @ralekna @schfkt @rokoroku @dankraus @wanganjun @rafaelkallis @aconanlai @zaphoyd @TheWillG @SimonSchick @afharo @lenovouser @AnandChowdhary @myovchev @RecuencoJones - please review this PR in the next few days. Be sure to explicitly select If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead. |
👋 Hi there! I’ve run some quick measurements against master and your PR. These metrics should help the humans reviewing this PR gauge whether it might negatively affect compile times or editor responsiveness for users who install these typings. Let’s review the numbers, shall we? Comparison details 📊
It looks like nothing changed too much. I won’t post performance data again unless it gets worse. |
@@ -1918,7 +1918,8 @@ declare namespace Joi { | |||
/** | |||
* Generates a schema object that matches an object data type (as well as JSON strings that have been parsed into objects). | |||
*/ | |||
object<TSchema = any>(schema?: SchemaMap<TSchema>): ObjectSchema<TSchema>; | |||
// tslint:disable-next-line:no-unnecessary-generics | |||
object<TSchema = any, T = TSchema>(schema?: SchemaMap<T>): ObjectSchema<TSchema>; |
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.
I think it would be better to add type argument to the extension method (ObjectSchema.keys()
), not to the definition method(Joi.object()
).
e.g.
interface ObjectSchema<TSchema = any> extends AnySchema {
/**
* Sets or extends the allowed object keys.
*/
keys(schema?: SchemaMap<TSchema>): this;
keys<TExtension>(schema: SchemaMap<TExtension>): ObjectSchema<TSchema & TExtension>;
}
and maybe using Overwrite<T, U>
type as the return(extended) type is a good idea I think.
reference: microsoft/TypeScript#12215 (comment)
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.
Good Idea. I actually spent some time to implement this but due to typescript type inference user should provide the Schema generic parameter on every validator extension in order to force TS to use his/her types. IMO defining all possible future fields in the Joi.object
is a safer pattern.
In other words I prefer this:
interface User {
name: string;
family: string;
age: number;
}
Joi.object<User>({ name: Joi.string() })
.keys({
family: Joi.string(),
})
.keys({
age: Joi.number(),
});
over this:
Joi.object<{ name: string }>({ name: Joi.string() })
.keys<{ family: string }>({
family: Joi.string(),
})
.keys<{ age: number }>({
age: Joi.number(),
});
After 5 days, no one has reviewed the PR 😞. A maintainer will be reviewing the PR in the next few days and will either merge it or request revisions. Thank you for your patience! |
I just published |
npm test
.)npm run lint package-name
(ortsc
if notslint.json
is present).This fixes #41128, @SimonSchick.