Return previous schema type #143
Conversation
| export type EnvSchemaOpt<T = EnvSchemaData> = { | ||
| schema?: JSONSchemaType<T> | AnySchema; | ||
| export type EnvSchemaOpt = { | ||
| schema?: object; |
There was a problem hiding this comment.
Did you evaluate adding the fluent-schema's JSONSchema type?
There was a problem hiding this comment.
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.
Hmm. object is not a very good type. Should use something like Record<string, any> or so.
There was a problem hiding this comment.
And to be honest i dont understand why we remove the generic type
There was a problem hiding this comment.
Because it is not compatible to different JSON schema library.
There was a problem hiding this comment.
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.
Well, it can never be false, because you cast it to EnvSchemaOpt in line 50.
|
@klaseca Any thoughts on review comments? |
mcollina
left a comment
mcollina
left a comment
There was a problem hiding this comment.
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 |
Not exactly. Not only |
|
attn: @fastify/typescript |
There was a problem hiding this comment.
Pull Request Overview
This PR updates the schema type handling to return the previous schema type for enhanced compatibility with fluent-json-schema. Key changes include:
- Adding tests for fluent-json-schema support in types/index.test-d.ts.
- Updating type annotations in tests to require explicit generic parameters.
- Modifying README examples to reflect the updated type usage.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| types/index.test-d.ts | Added fluent-json-schema tests and modified type annotations. |
| README.md | Updated example to include explicit generic parameter usage in envSchema. |
7 participants
Checklist
npm run testandnpm run benchmarkand the Code of conduct
Description:
As discussed in #138,
env-schemashould supportfluent-json-schemalibrary out-of-the-box (without explicitly callingvalueOfmethod). Given this requirement, we cannot use a narrower type for schema (reasons why we cannot use code fromfluent-json-schemaare described in #137)In future, if we remove this requirement, it will be possible to return a more strict type