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

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

Closed
merodiro opened this issue Aug 25, 2024 · 11 comments
Closed

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

merodiro opened this issue Aug 25, 2024 · 11 comments
Assignees
Labels
bug Something isn't working priority This has priority workaround Workaround fixes problem

Comments

@merodiro
Copy link

merodiro commented Aug 25, 2024

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
Copy link
Owner

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 fabian-hiller self-assigned this Aug 25, 2024
@fabian-hiller fabian-hiller added bug Something isn't working priority This has priority labels Aug 25, 2024
@fabian-hiller fabian-hiller changed the title forward breaks pipe in v0.0.38+ forward breaks pipe in >=v0.38.x Aug 25, 2024
@fabian-hiller
Copy link
Owner

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'],
    ),
  ),
]);

@fabian-hiller fabian-hiller added the workaround Workaround fixes problem label Aug 25, 2024
@Andarist
Copy link

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
Copy link
Contributor

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
Copy link

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

@Andarist
Copy link

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

@fabian-hiller
Copy link
Owner

fabian-hiller commented Aug 26, 2024

@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
Copy link
Contributor

@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.
3. Empty the body of the _addIssue function.
4. Remove BaseMetadata from InferInput, InferOutput, InferIssue and PipeAction.
5. Remove the last optional parameter from the _addIssue function.
6. Simplify the PathKeys type.

@fabian-hiller
Copy link
Owner

I will try to investigate this today.

@fabian-hiller
Copy link
Owner

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
Copy link
Owner

v0.41.0 is available

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority This has priority workaround Workaround fixes problem
Projects
None yet
Development

No branches or pull requests

5 participants