-
Notifications
You must be signed in to change notification settings - Fork 25
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
Return previous schema type #143
base: master
Are you sure you want to change the base?
Conversation
export type EnvSchemaOpt<T = EnvSchemaData> = { | ||
schema?: JSONSchemaType<T> | AnySchema; | ||
export type EnvSchemaOpt = { | ||
schema?: object; |
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.
Did you evaluate adding the fluent-schema's JSONSchema
type?
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 am not quite sure what you mean by that. Are you suggesting to add JSONSchema
type from fluent-json-schema
to union type for schema? If so, read description of this pull request
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.
Hmm. object is not a very good type. Should use something like Record<string, any> or so.
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.
And to be honest i dont understand why we remove the generic type
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.
Because it is not compatible to different JSON schema library.
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 you should read backstory of this pull request - #134 , #137 , #138 . I think it will become clear to you
As for typescript
:
Record<string, any>
and object
are equal. Also, we can not use Record<string, unknown>
type, because it will not work with interfaces (typescript playground)
const optWithSchemaFluent: EnvSchemaOpt = { | ||
schema: schemaFluent, | ||
}; | ||
expectType<EnvSchemaOpt>(optWithSchemaFluent); |
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.
Well, it can never be false, because you cast it to EnvSchemaOpt in line 50.
@klaseca Any thoughts on review comments? |
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'm not really convinced this is a good idea. It seems it worsen the API surface.
The PR is long enough to forget the detail on why it happen. Even |
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.
lgtm
Not exactly. Not only |
attn: @fastify/typescript |
Checklist
npm run test
andnpm run benchmark
and the Code of conduct
Description:
As discussed in #138,
env-schema
should supportfluent-json-schema
library out-of-the-box (without explicitly callingvalueOf
method). Given this requirement, we cannot use a narrower type for schema (reasons why we cannot use code fromfluent-json-schema
are described in #137)In future, if we remove this requirement, it will be possible to return a more strict type