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

Zod resolved type change for Opaque type between v3.14.4 -> v3.15.1 #1121

Closed
bluepnume opened this issue May 8, 2022 · 5 comments
Closed

Comments

@bluepnume
Copy link

bluepnume commented May 8, 2022

I have the following code:

type Opaque<Type, Token = unknown> = Type & { tag: Token }; 

enum FACE {
    QUEEN = 'QUEEN',
    KING = 'KING'
}

enum SUIT {
    HEARTS = 'HEARTS',
    DIAMONDS = 'DIAMONDS'
}

type FaceCard<Face extends FACE> = Opaque<`${ Face }_${ SUIT }`, 'FaceCard'>;

type Hand = z.output<
    z.ZodObject<{
        card : z.ZodType<FaceCard<FACE.QUEEN>, z.ZodTypeDef, string>,
    }>
>;

In zod@3.14.4, Hand resolved to:

type Hand = {
    card: FaceCard<FACE.QUEEN>;
}

But now in zoid@3.15.1, Hand is resolving to:

type Hand = {
    card: (FaceCard<FACE.QUEEN> | undefined) & FaceCard<FACE.QUEEN>;
}

Why did the second type get more complex here, when the first one is correct?

(By the way -- incredible library. Thank you for making this, seriously.)

bentefay added a commit to bentefay/zod that referenced this issue May 9, 2022
Fixes this issue: colinhacks#1121

Specifically, `{ property: (T | undefined) & T }` does not seem to
flatten to `{ property: T }` in the case that T is a complex string literal.
bentefay added a commit to bentefay/zod that referenced this issue May 9, 2022
Fixes this issue: colinhacks#1121

Specifically, `{ property: (T | undefined) & T }` does not seem to
flatten to `{ property: T }` in the case that T is a complex string literal.
@bentefay
Copy link
Contributor

bentefay commented May 9, 2022

Hi @bluepnume.

Sorry about this. I changed the definition of z.ZodObject in my PR (#1117) that was recently merged. Its behaviour was unchanged in all the cases I tested, but it looks like you found a case that resolves differently. I've made a PR that fixes the type to preserve the existing behaviour for your example (#1122).

As a side node, although the type looks ugly and we should definitely fix the problem, I believe the types { property: (T | undefined) & T } and { property: T } are technically the same, so hopefully you shouldn't be seeing any issues with using the type until my PR is merged. Is that right?

@bluepnume
Copy link
Author

Thanks @bentefay! Appreciate the fast turnaround!

No huge rush to merge, since I can just lock to 3.14.4 for now. But since you asked, the issue that this causes for me is demonstrated with the following code:

type QueenCard = Hand['card'];

type CardValue = {
    [ FACE.QUEEN ] : 12,
    [ FACE.KING ] : 13,
};

type CardToValue<Face extends FACE> = Face extends keyof CardValue
    ? CardValue[Face]
    : never;

const cardToValue = <Face extends FACE>(card : FaceCard<Face>) : CardToValue<Face> => {
    return someLogic(card);
};

const value = cardToValue('' as QueenCard);

const expectedValue : 12 = value;

console.log('Value is', value, 'expected', expectedValue);

^ This code type-checks with 3.14.4. But with 3.15.1, I get:

const expectedValue : 12 = value;
Type '12 | 13' is not assignable to type '12'.

-- essentially, while (T | undefined) & T and T are equivalent, you can use T as an index but you can't with (T | undefined) & T, so this type refinement breaks other parts of my code that rely on that.

@bentefay
Copy link
Contributor

bentefay commented May 9, 2022

Ahh that makes complete sense. It's also not something I could have possibly dreamed up as a test 😄. Now I see it though I suppose we could add that as a unit test, though it would be perhaps a little too specific? I wonder if there is a simpler set of types that expose the problem.

colinhacks pushed a commit that referenced this issue May 12, 2022
* Fix addQuestionsMarks so it resolves correctly in more cases

Fixes this issue: #1121

Specifically, `{ property: (T | undefined) & T }` does not seem to
flatten to `{ property: T }` in the case that T is a complex string literal.

* Ran lint on languageServerFeatures.test.ts
@colinhacks
Copy link
Owner

Resolved in 3.16, thanks @bentefay

@bluepnume
Copy link
Author

Thanks @bentefay and @colinhacks! And again thanks for a stellar library, you guys rock.

MrAwesome pushed a commit to MrAwesome/zod that referenced this issue May 20, 2022
…1122)

* Fix addQuestionsMarks so it resolves correctly in more cases

Fixes this issue: colinhacks#1121

Specifically, `{ property: (T | undefined) & T }` does not seem to
flatten to `{ property: T }` in the case that T is a complex string literal.

* Ran lint on languageServerFeatures.test.ts
StarDev930 pushed a commit to StarDev930/zod that referenced this issue Jan 29, 2023
* Fix addQuestionsMarks so it resolves correctly in more cases

Fixes this issue: colinhacks/zod#1121

Specifically, `{ property: (T | undefined) & T }` does not seem to
flatten to `{ property: T }` in the case that T is a complex string literal.

* Ran lint on languageServerFeatures.test.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants