fabian-hiller / valibot

The modular and type safe schema library for validating structural data 🤖
https://valibot.dev
MIT License
6.35k stars 204 forks source link

`forward` breaks `pipe` in >=v0.38.x #799

Closed merodiro closed 2 months ago

merodiro commented 3 months ago

The following worked in 0.0.37 but stopped working now

Playground URL

import * as v from "valibot";

v.variant("type", [
  v.pipe(
    v.object({
      type: v.literal("email"),
      email: v.pipe(v.string(), v.email()),
      confirm_email: v.pipe(v.string(), v.email()),
    }),
    v.forward(
      v.partialCheck(
        [["email"], ["confirm_email"]],
        (input) => input.email === input.confirm_email,
        "Email mismatch"
      ),
      ["confirm_email"]
    )
  ),
  v.pipe(
    v.object({
      type: v.literal("phone"),
      phone: v.string(),
      confirm_phone: v.string(),
    }),
    v.forward(
      v.partialCheck(
        [["phone"], ["confirm_phone"]],
        (input) => input.phone === input.confirm_phone,
        "Phone mismatch"
      ),
      ["confirm_phone"]
    )
  ),
]);

I'm not sure forward is the problem or if variant doesn't support forward or if there is something wrong with my code

fabian-hiller commented 3 months ago

This is a really strange bug. It does not seem to happen for the first occurrence of forward and partialCheck, but for every occurrence after that. Looks like a TypeScript bug to me, but I need more time to investigate.

fabian-hiller commented 3 months ago

I have no idea what's going on here. Seems like TypeScript's inference is screwing something up in a very strange way. @Andarist, just in case you have some time... do you have any idea what's happening here?

In the meantime, anyone encountering this problem can downgrade to v0.37.0 or use type casting to explicitly tell TypeScript what to do here. See this playground.

import * as v from 'valibot';

v.variant('type', [
  v.pipe(
    v.object({
      type: v.literal('email'),
      email: v.pipe(v.string(), v.email()),
      confirm_email: v.pipe(v.string(), v.email()),
    }),
    v.forward(
      v.partialCheck(
        [['email'], ['confirm_email']],
        (input) => input.email === input.confirm_email,
        'Email mismatch',
      ) as v.GenericValidation<{
        type: 'email';
        email: string;
        confirm_email: string;
      }>,
      ['confirm_email'],
    ),
  ),
  v.pipe(
    v.object({
      type: v.literal('phone'),
      phone: v.string(),
      confirm_phone: v.string(),
    }),
    v.forward(
      v.partialCheck(
        [['phone'], ['confirm_phone']],
        (input) => input.phone === input.confirm_phone,
        'Phone mismatch',
      ) as v.GenericValidation<{
        type: 'phone';
        phone: string;
        confirm_phone: string;
      }>,
      ['confirm_phone'],
    ),
  ),
]);
Andarist commented 3 months ago

I can help to debug this but I'd have to get a minimal repro case of the problem. Ideally, both the user's code and te valibot's code would be distilled to the bare minimum.

EltonLobo07 commented 3 months ago

I think it's related to the forward action's type inference. If the forward action is given an almost similar check or partialCheck action (these are the actions I have tried using, there may be more cases), one of the forward action's type inference stops working. Do you think the code block below is enough to debug?

import * as v from "valibot";

const K1Schema = v.pipe(
  v.object({k1: v.string(), confirm_k1: v.string()}),
  v.forward(
    v.partialCheck(
      [["k1"], ["confirm_k1"]], 
      input => input.k1 === input.confirm_k1
    ), ["confirm_k1"]
  )
);

const K2Schema = v.pipe(
  v.object({k2: v.string(), confirm_k2: v.string()}),
  // error here
  v.forward(
    v.partialCheck(
      [["k2"], ["confirm_k2"]],
      input => input.k2 === input.confirm_k2
    ), ["confirm_k2"]
  )
);

The code can be further reduced. It might not exactly replicate how the user has used the APIs and the example might not make sense but it still highlights the problem. Adding the reduced code below.

import * as v from "valibot";

const K1Schema = v.pipe(
  v.object({k1: v.string()}),
  v.forward(v.check(input => input.k1.includes("k1")), ["k1"])
);

const K2Schema = v.pipe(
  v.object({k2: v.string()}),
  // error here
  v.forward(v.check(input => input.k2.includes("k2")), ["k2"])
);
ruiaraujo012 commented 3 months ago

I'm facing the same issue. Here is my playground.

Andarist commented 3 months ago

@EltonLobo07 thanks, could you also inline the relevant functions from valibot to create a self-contained repro?

fabian-hiller commented 3 months ago

@EltonLobo07 if you have the time, feel free to remove Valibot's import by adding the functions and types directly to a TS playground and then start removing and simplifying things until you reach the bare minimum to reproduce the problem. If you don't have the time, I will try to do this in the next few days.

EltonLobo07 commented 3 months ago

@fabian-hiller I have copied the necessary Valibot functions and types, along with the example that generates the type error here. I have made some simplifications here. Can you take a look? Maybe you can simplify the code further? If you think the simplifications were too much, you can use the first playground as the starting point. Also, to reduce the Valibot types and functions needed, I have used a different example in the playgrounds.

Here are the changes made in the second playground: 1. Remove ArrayPathItem, MapPathItem, SetPathItem and ObjectPathItem from IssuePathItem type. 2. Empty the body of the pipe function and set the return type to void.

  1. Empty the body of the _addIssue function.
  2. Remove BaseMetadata from InferInput, InferOutput, InferIssue and PipeAction.
  3. Remove the last optional parameter from the _addIssue function.
  4. Simplify the PathKeys type.
fabian-hiller commented 2 months ago

I will try to investigate this today.

fabian-hiller commented 2 months ago

My investigation showed that the source of this problem in Valibot is the recursive reference property of the BaseValidation type. I haven't investigated why TypeScript behaves wired in this case, as I have less time at the moment and don't think this is completely solvable with changes on our end.

Changing the return type from BaseValidation<TInput, TOutput, TIssue> to BaseValidation<unknown, unknown, BaseIssue<unknown>> fixed the problem, but to be consistent across our codebase I prefer to make the same changes to our BaseTransformation and BaseMetadata types. For BaseTransformation this introduced new TypeScript errors, so in the end any was the only solution and to be consistent I've made this change for all action base types. Since the reference property of all these types is not too important for the type safety of this library, I think it is best for now to make it less strict to be able to move forward with our v1 RC release. However, feel free to give me feedback or investigate this problem further.

fabian-hiller commented 2 months ago

v0.41.0 is available