Repository navigation
Conversation
Signed-off-by: Mohd Salauddin <sallumalik1111@gmail.com>
ilteoood
reviewed
Oct 10, 2026
|
|
||
| type EnvSchema = typeof envSchema | ||
|
|
||
| type FluentSchema = { |
Member
There was a problem hiding this comment.
- why it needs to change:
fluent-json-schema's own README documentsisFluentSchemaas the public detection flag;isFluentJSONSchemais also set on everyBaseSchema<T>but is not the documented public contract. The two-field structural type is broader than what the upstream docs promise to set. - code suggestion:
type FluentSchema = { isFluentSchema: boolean }
- why the suggestion differs: narrows the structural requirement to the single documented public flag; an object with
isFluentSchema: booleanis a fluent schema per the upstream README, and the second field does not add coverage the documented contract guarantees.
|
|
||
| export type EnvSchemaOpt<T = EnvSchemaData> = { | ||
| schema?: JSONSchemaType<T> | AnySchema; | ||
| schema?: JSONSchemaType<T> | AnySchema | FluentSchema; |
Member
There was a problem hiding this comment.
- why it needs to change: this PR changes the types only. The runtime detection in
index.jsstill usesSymbol.for('fluent-schema-object')+.valueOf(). The structural type is a type-level approximation of what the runtime already accepts, not a new contract, and a future reader of this line could infer that adding a field here changes runtime behavior. - code suggestion: add a short
// type-only: runtime uses Symbol.for('fluent-schema-object') in index.jsabove the union, or attach a JSDoc to theFluentSchematype pointing at the runtime path so the two stay in sync. - why the suggestion differs: makes the type-only nature explicit at the point of use; prevents a future contributor from "tightening" the type to match a runtime field they think is checked and breaking valid fluent schemas.
| const envSchemaTypebox = envSchema<SchemaTypebox>({ schema: schemaTypebox }) | ||
| expect(envSchemaTypebox).type.toBe<SchemaTypebox>() | ||
|
|
||
| const envSchemaFluent = envSchema<EnvData>({ |
Member
There was a problem hiding this comment.
- why it needs to change: this assertion uses
S.object<EnvData>()to force the generic T, butfluent-json-schemadoes not infer T from.prop(...)arguments (unlike TypeBox). Users will not naturally type fluent schemas this way, and the test reads as promising more type inference than fluent-json-schema delivers. - code suggestion: either drop this assertion (the
optWithFluentSchemablock at line 50 already proves the regression fix), or replace it with a.valueOf()assertion that locks in the documented escape hatch from Use with fluent-json-schema now requires to call valueOf in ObjectSchema when using Typescript #138, e.g.expect(S.object().valueOf as unknown as object).type.toBeAssignableTo<JSONSchemaType<EnvSchemaData>>(). If the generic-T path must be exercised, leave a one-line comment that it is a manual-typing witness, not runtime validation. - why the suggestion differs: removes the over-claim about fluent-json-schema's type inference; the replacement locks in the workaround the original issue explicitly mentioned, and the regression is still proven by the first assertion.
Puppo
reviewed
Oct 10, 2026
| type EnvSchema = typeof envSchema | ||
|
|
||
| type FluentSchema = { | ||
| isFluentSchema: boolean; |
Member
There was a problem hiding this comment.
Hi @salluexez, thanks for your PR.
I'm not sure about this.
It seems like a Fluent workaround, not really an issue with this package.
In this branch, https://github.com/Puppo/env-schema/tree/test-fluent-schema, I prove that we should reach the same result without touching the codebase.
I'm happy to let you review your pr to add these tests if you'd like.
If you proceed, please also remember to add the test example to the JavaScript tests.
Comment on lines
+50
to
+53
| const optWithFluentSchema: EnvSchemaOpt = { | ||
| schema: S.object().prop('PORT', S.number().default(3000).required()), | ||
| } | ||
| expect(optWithFluentSchema).type.toBe<EnvSchemaOpt>() |
Member
There was a problem hiding this comment.
Suggested change
| const optWithFluentSchema: EnvSchemaOpt = { | |
| schema: S.object().prop('PORT', S.number().default(3000).required()), | |
| } | |
| expect(optWithFluentSchema).type.toBe<EnvSchemaOpt>() | |
| const optWithFluentSchema = { | |
| schema: S.object().prop('PORT', S.number().default(3000).required()), | |
| } | |
| expect(optWithFluentSchema).type.toBeAssignableTo<EnvSchemaOpt>() |
| schema: S.object<EnvData>().prop( | ||
| 'PORT', | ||
| S.number().default(3000).required() | ||
| ), |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Allow
fluent-json-schemaschemas to be passed toenvSchemawithout TypeScript errors.Problem
env-schemasupports Fluent Schema objects at runtime and documents that usage, butEnvSchemaOpt.schemaonly accepted Ajv schema types. Consequently, valid Fluent Schema objects were rejected by TypeScript.Solution
Add a dependency-free structural
FluentSchematype and include it in the accepted schema union. This preserves existing Ajv and TypeBox support without adding a production or peer dependency.Testing
npm run lintnpm test— 46 unit tests and 23 type assertions passed; 100% runtime coveragenpm pack --dry-run --cache /tmp/env-schema-npm-cachegit diff --checkScreenshots / Evidence
Not applicable; this is a TypeScript declaration fix.
Related Issue
Closes #138
Additional Notes
The regression test fails before the fix with TS2322 and passes afterward. There are no runtime changes or compatibility changes for existing schema types.