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

Strict object schema with a boolean property expects alternative, not boolean #2848

Closed
joshkel opened this issue Sep 26, 2022 · 9 comments · Fixed by #2851
Closed

Strict object schema with a boolean property expects alternative, not boolean #2848

joshkel opened this issue Sep 26, 2022 · 9 comments · Fixed by #2851
Assignees
Labels
bug Bug or defect support Questions, discussions, and general support types TypeScript type definitions
Milestone

Comments

@joshkel
Copy link
Contributor

joshkel commented Sep 26, 2022

Support plan

  • is this issue currently blocking your project? (yes/no): no
  • is this issue affecting a production system? (yes/no): no

Context

  • node version: 14.19.3
  • module version with issue: 17.6.1
  • last module version without issue: 17.6.0
  • environment (e.g. node, browser, native): Node
  • used with (e.g. hapi application, another framework, standalone, ...): Hapi
  • any other relevant information: TypeScript 4.8.2

What are you trying to achieve or the steps to reproduce?

Joi 17.6.1's addition of AlternativesSchema for a strict object map has caused TypeScript to start reporting an error for boolean properties.

interface Test {
  enabled: boolean;
}

Joi.object<Test, true>({
  enabled: Joi.boolean(),
});

What was the result you got?

error TS2739: Type 'BooleanSchema' is missing the following properties from type 'AlternativesSchema': conditional, match, try

What result did you expect?

Successful compilation.

The problem appears to be that IsUnion returns true for booleans. I think that this may be because TypeScript treats boolean as the union of true and false (see the mentions of 'boolean' within checker.ts), but I'm really not sure. See also Stack Overflow's discussion on the limitations of an IsUnion implementation.

To fix it, maybe reorder ObjectPropertiesSchema to check boolean before union?

TypeScript playground link for experimenting with IsUnion implementations

(I'm not sure about ObjectPropertiesSchema's union check for another reason: It thinks that interface Test { value: 'a' | 'b' } should use an AlternativesSchema, but it seems like a more idiomatic Joi implementation would be Joi.string().valid('a', 'b'). That's a separate issue and not currently affecting me, but I wanted to mention it in case it affects how this issue should be addressed. Thanks!)

@joshkel joshkel added the support Questions, discussions, and general support label Sep 26, 2022
@matgott
Copy link

matgott commented Sep 28, 2022

Same issue here.

@Marsup
Copy link
Collaborator

Marsup commented Sep 28, 2022

@matgott in general, please avoid doing that, "+1 comments" are not bringing new information nor helping projects prioritize issues. Use GitHub's 👍🏻 reaction if you want to vote for issues instead.

@Marsup
Copy link
Collaborator

Marsup commented Sep 28, 2022

@joshkel I've tried bringing it right after the boolean check, it then breaks the alternatives case, still looking into possible solutions but it seems tricky.

@Marsup Marsup self-assigned this Sep 28, 2022
@Marsup Marsup added this to the 17.6.2 milestone Sep 28, 2022
@Marsup Marsup added bug Bug or defect types TypeScript type definitions labels Sep 28, 2022
@Marsup
Copy link
Collaborator

Marsup commented Sep 28, 2022

@joshkel I think I've found a way that works for all cases (see #2850), can you check in your project if that causes other regressions elsewhere?

@wodCZ
Copy link

wodCZ commented Sep 29, 2022

I'm getting into the same issue with union strings:

export interface ConfigSchema {
  NODE_ENV: 'development' | 'production' | 'test';
}


export const schema = Joi.object<ConfigSchema, true>({
  NODE_ENV: Joi.string()
    .valid('development', 'production', 'test')
    .default('production'),
// ^ TS2739: Type 'StringSchema' is missing the following properties from type 'AlternativesSchema': conditional, match, try
});

But maybe I'm using it wrong 🤷‍♂️ I didn't expect a breaking change in a patch release, so I can't dig deeper now.

@Marsup
Copy link
Collaborator

Marsup commented Sep 29, 2022

Your use case seems valid, maybe I'll revert that change until there's a reliable solution 🤔

joshkel added a commit to joshkel/joi that referenced this issue Sep 29, 2022
The goal is to handle "complex" unions (unions that aren't merely subsets of a single primitive type) without interfering with simple enumerated-constant-style unions (such as `'development' | 'production' | 'test'`) or booleans.

Fixes hapijs#2848
@joshkel
Copy link
Contributor Author

joshkel commented Sep 29, 2022

I have an attempted fix at #2851. I'm really not sure if this works for all cases and is the best approach, but it seems to cover both my booleans and the 'development' | 'production' | 'test' constant from @wodCZ.

@Marsup
Copy link
Collaborator

Marsup commented Sep 29, 2022

That was going to be my next attempt, thanks a lot, I think that works.

@Marsup
Copy link
Collaborator

Marsup commented Sep 29, 2022

New version is published.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug or defect support Questions, discussions, and general support types TypeScript type definitions
Projects
None yet
4 participants