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

MappedTypeNodeParser doesn't take Unions into account #2106

Open
AmadeusK525 opened this issue Nov 5, 2024 · 2 comments
Open

MappedTypeNodeParser doesn't take Unions into account #2106

AmadeusK525 opened this issue Nov 5, 2024 · 2 comments

Comments

@AmadeusK525
Copy link

AmadeusK525 commented Nov 5, 2024

Mapped types in TypeScript accept Unions as the constraint type, and the resulting output (in TS) will be similar to Distributive Conditional Types. Take the following code, for example:

type A = {
    a: string;
};

type B = {
    b: number;
};

type MappedUnion<T extends object> = {
    [P in keyof T]: number;
};

export type MyObject = MappedUnion<A | B>;

The resulting type would still be a Union, but with a: string being mapped to a: number, because the mapping is applied to each Union case individually (the LSP shows MyObject to equal MappedUnion<A> | MappedUnion<B>).

The problem is that the MappedTypeNodeParser doesn't handle this correctly, as this case is not covered at all. It generates the constraintType and keyListType (confusing names) by passing mappedNode.typeParameter.constraint to a child parser. The parser will be a TypeOperatorNodeParser, which will, in turn, return all of the possible keys for that type parameter (T). When T is a Union, though, it will return a flat list for its keys. Going back to MappedTypeNodeParser, there's no differentiation if that flat list of keys comes from a Union or an object, it will just assume that it's an object and construct the mapped type:

        if (keyListType instanceof UnionType) {
            // Key type resolves to a set of known properties
            return new ObjectType(
                id,
                [],
                this.getProperties(node, keyListType, context),
                this.getAdditionalProperties(node, keyListType, context),
            );
        }

This means that instead of getting a schema like this for the original example:

"MyObject": {
  "anyOf": [
    {
      "type": "object",
      "properties": {
        "a": {
          "type": "number"
        },
      }
      "required": ["a"]
    },
    {
      "type": "object",
      "properties": {
        "a": {
          "type": "number"
        },
      }
      "required": ["a"]
    }
  ]
}

we get this:

"MyObject": {
  "type": "object",
  "properties": {
    "a": {
      "type": "number"
    },
    "b": {
      "type": "number"
    }
  },
  "required": ["a", "b"]
}

(it, by mistake, turns it into an Intersection by using the flat list of keys for that Union as properties for a new object)

@arthurfiorette
Copy link
Collaborator

Hi! thanks for opening this detailed issue request! Are you up to PR this change? remember to add a unit test! (Your example can be used as unit test)

@AmadeusK525
Copy link
Author

AmadeusK525 commented Nov 6, 2024

Hey @arthurfiorette thanks for responding! Unfortunately I don't have the time to really dive into and understand the code right now, I did try some stuff but the Context handling that you guys use is a little more complex than I initially thought. I did start out by making the unit test 😛 but I won't be able to fix this in the coming weeks

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

2 participants