Skip to content
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

Use with fluent-json-schema now requires to call valueOf in ObjectSchema when using Typescript #138

Open
2 tasks done
Ceres6 opened this issue Nov 17, 2022 · 7 comments
Open
2 tasks done
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@Ceres6
Copy link

Ceres6 commented Nov 17, 2022

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the regression has not already been reported

Last working version

5.1.0

Stopped working in version

5.1.1

Node.js version

18.x

Operating system

macOS

Operating system version (i.e. 20.04, 11.3, 10)

12.3

💥 Regression Report

Typings in Typescript now throw an error when using a fluent-json-schema generated schema as input for envSchema

Steps to Reproduce

In a Typescript file the next block of code reproduces the issue

import envSchema from 'env-schema'
import S from 'fluent-json-schema'

envSchema({
  schema: S.object()
})
Type 'ObjectSchema' is not assignable to type 'AnySchema | UncheckedJSONSchemaType<EnvSchemaData, false> | undefined'.
  Type 'ObjectSchema' is not assignable to type '{ type: "object"; additionalProperties?: boolean | UncheckedJSONSchemaType<unknown, false> | undefined; unevaluatedProperties?: boolean | UncheckedJSONSchemaType<unknown, false> | undefined; ... 7 more ...; maxProperties?: number | undefined; } & { ...; } & { ...; } & { ...; }'.
    Property 'type' is missing in type 'ObjectSchema' but required in type '{ type: "object"; additionalProperties?: boolean | UncheckedJSONSchemaType<unknown, false> | undefined; unevaluatedProperties?: boolean | UncheckedJSONSchemaType<unknown, false> | undefined; ... 7 more ...; maxProperties?: number | undefined; }'.ts(2322)

Nevertheless the following block of code does not throw an error

import envSchema from 'env-schema'
import S from 'fluent-json-schema'

envSchema({
  schema: S.object().valueOf
})

Expected Behavior

As ObjectSchema from fluent-json-schema was removed from typings in #137 it might be solved otherwise or maybe be documented to avoid future confusion

@climba03003
Copy link
Member

cc @klaseca

I think I would revert all those changes. It cannot support all the third-party library. Especially, it cannot support fluent-json-schema.

@klaseca
Copy link
Contributor

klaseca commented Nov 17, 2022

Under the hood, valueOf is called when passing a schema from fluent-json-schema. How about we remove this code and explicitly call valueOf when passing the schema? It seems to me that if env-schema should work not only with fluent-json-schema, then there should be no code specifically for this library

@climba03003
Copy link
Member

climba03003 commented Nov 17, 2022

How about we remove this code and explicitly call valueOf when passing the schema?

No, it should support out-of-the-box. Since, fastify itself do the same.
cc @fastify/core WDYT?

Anyway, it would be a breaking change in either end. If no other fixes come up, the only choice is revert on current release.

@Uzlopak
Copy link
Contributor

Uzlopak commented Nov 17, 2022

I would also revert the problematic PR and maybe add fluent-json-schema as dependency or as peerDependency

@klaseca
Copy link
Contributor

klaseca commented Nov 17, 2022

Perhaps the best solution in this situation would be to return the object type for the schema. At the same time, leave the re-import of the JSONSchemaType type from the ajv package. In this case, we will return the previous fully working state and leave the option to declare a type-checked schema

@mcollina
Copy link
Member

Fastify can support fluent-json-schema without the need of it being a dependency. Take a look how it's done there.

@Eomm Eomm added bug Something isn't working good first issue Good for newcomers labels Nov 24, 2022
@jessekrubin
Copy link

FWIW: Something similar is going on with types here when using with typebox

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

7 participants