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

Add a fallback override for pipe method to accept unlimited pipe items #643

Open
jindong-zhannng opened this issue Jun 11, 2024 · 24 comments
Assignees
Labels
enhancement New feature or request question Further information is requested

Comments

@jindong-zhannng
Copy link

Current behavior

pipe method can only accept up to 9 pipe items because of its type signature.

It's enough in most cases, but could be frustrating in some rare cases.

Such as a very long pipeline:

const schema = pipe(
  string(),
  trim(),
  minLength(1),
  decimal(),
  transform(parseInt),
  number(),
  minValue(1),
  maxValue(100),
  finite(),
  integer(),
  // ... errors start from here
);

Or build pipe items dynamically:

const pipeItems = buildItems()
const schema = pipe(string(), ...pipeItems) // error

Expected behavior

Have a fallback override that can accept unlimited items.

Inexact type inference is acceptable if difficulty exists. e.g. unknown output type.


BTW, I am happy to contribute codes but was facing difficulties in writing types.

If someone can give a hand I will raise a pull request soon.

@fabian-hiller fabian-hiller self-assigned this Jun 11, 2024
@fabian-hiller fabian-hiller added enhancement New feature or request question Further information is requested labels Jun 11, 2024
@fabian-hiller
Copy link
Owner

Thanks for creating this issue. I understand your point, but I am not sure if we should change pipe or if it is even possible to type it correctly while keeping the current type inference for its arguments. Even if there is a theoretical limit of 9 arguments, this can easily be bypassed by nesting multiple pipelines. This works because pipe simply returns a modified schema that can be passed as the first argument to another pipe call.

const Schema = v.pipe(v.pipe(v.string(), ...), ...);

The function signature you are looking for is probably the function signature of the current overload implementation. I am open to investigating if this works and makes sense for dynamically creating pipelines.

export function pipe<
  const TSchema extends BaseSchema<unknown, unknown, BaseIssue<unknown>>,
  const TItems extends PipeItem<unknown, unknown, BaseIssue<unknown>>[],
>(...pipe: [TSchema, ...TItems]): SchemaWithPipe<[TSchema, ...TItems]>;

@jindong-zhannng
Copy link
Author

Thanks for your explanation. For reference, my use case is similar to case 2 I mentioned above:

I will build pipe items dynamically at first, and would like to assemble them to the final schema later. I'm stuck at this step because of this type limitation.

const validationComponents = someBuilder()
const schema = pipe(
  validationComponents.baseSchema,
  ...validationComponents.pipeItems, // ts error: A spread argument must either have a tuple type or be passed to a rest parameter.
)

In this scenario I don't really care about the precision of type inference. Because I already lose such type information of valibot actions at the first step. So I'm just expecting a generic type here.

OTOH, if the nesting pipelines are completely equivalent to a single pipe call with multiple items, it may be a valuable workaround for my case.

@fabian-hiller
Copy link
Owner

Can you share the code of someBuilder? It may be possible to make it type safe. In the meantime, you can add a // @ts-expect-error comment to ignore the TS error.

@jindong-zhannng
Copy link
Author

Can you share the code of someBuilder? It may be possible to make it type safe.

Because the data source is dynamic, it's impossible to analyze the number of built pipe items statically.

you can add a // @ts-expect-error comment to ignore the TS error.

I will try nesting pipelines firstly. If works it will make things easier. Ignoring error is the last resort and nobody likes it.

@fabian-hiller
Copy link
Owner

Also, feel free to investigate if this overload signature would solve your problem. If so, I will consider adding it.

export function pipe<
  const TSchema extends BaseSchema<unknown, unknown, BaseIssue<unknown>>,
  const TItems extends PipeItem<unknown, unknown, BaseIssue<unknown>>[],
>(...pipe: [TSchema, ...TItems]): SchemaWithPipe<[TSchema, ...TItems]>;

@jindong-zhannng
Copy link
Author

It should be. Actually I have tried it but its type inference result is incomprehensible for me. I'm stuck at writing a passing type test for it.

@fabian-hiller
Copy link
Owner

Let me know if you think I can help you.

@dyljhd
Copy link

dyljhd commented Jun 24, 2024

I came across this nine pipe issue fairly recently, which was a little frustrating to deal with.
I saw you mentioned potential limitations with TS being the reasoning for the nine pipe item limit - correct me if I am wrong here.
If that is the case, I have come up with a type that could be used to infer the pipe item types instead of having to explicitly type all of the pipe functions from one item to nine items directly.
I am curious to see if this would be of any help to you @fabian-hiller

// If the first item is always enforced (in pipe function below), the empty array checks technically aren't needed 
// ...but this utility isn't aware of that constraint, so it is technically still needed here
type InferPipe<
    TSchema extends BaseSchema<unknown, unknown, BaseIssue<unknown>>, 
    TRestPipeItems extends PipeItem<unknown, unknown, BaseIssue<unknown>>[], 
    TPrevPipeItem extends PipeItem<unknown, unknown, BaseIssue<unknown>> | null = null;
> = {
    0: [];
    1: TRestPipeItems extends [
        infer CurrPipeItem extends PipeItem<unknown, unknown, BaseIssue<unknown>>, 
        ...infer RestPipeItems extends PipeItem<unknown, unknown, BaseIssue<unknown>>[]
    ]
        ? [
            PipeItem<InferOutput<TPrevPipeItem extends null ? TSchema : TPrevPipeItem>, InferOutput<CurrPipeItem>, InferIssue<CurrPipeItem>>, 
            ...InferPipe<TSchema, RestPipeItems, CurrPipeItem>
        ]
        : [];
}[TRestPipeItems extends [] ? 0 : 1];

// The tweaked pipe function (with only one pipe function needing to be defined):
function pipe<
  const TSchema extends BaseSchema<unknown, unknown, BaseIssue<unknown>>,
  // Enforce at least one item in the pipe
  const TItems extends [PipeItem<unknown, unknown, BaseIssue<unknown>>, ...PipeItem<unknown, unknown, BaseIssue<unknown>>[]]
>(
  schema: TSchema,
  ...items: TItems
): SchemaWithPipe<[TSchema, ...InferPipe<TSchema, TItems>]>;

// Example usage:
const schema = pipe(
  string(),
  minLength(1, 'Message'),
  maxLength(2, 'Message'),
);

Here are some of the TS outputs:
One pipe item being enforced:
image
An example output with a schema and two pipe items:
image
TS depth limit being hit at 31 pipe items (in this usage):
image
image
Passing arguments that aren't of a pipe item type:
image

@fabian-hiller
Copy link
Owner

Thank you for your research. Unfortunately, your implementation does not work for transformations. Also, the types are not correctly inferred. In your example, the resulting type of minLength(1, 'Message') is MinLengthAction<LengthInput, 1, 'Message'> instead of MinLengthAction<string, 1, 'Message'>.

How did you reach the 9 item limit? Can you share that scheme with me? We could extend the limit to more items.

@TeChn4K
Copy link

TeChn4K commented Jun 25, 2024

I reached the nine limit too : my schema contains a lot of forward+partialCheck

@dyljhd
Copy link

dyljhd commented Jun 25, 2024

No worries, thanks for the reply. Ahh yes, I see, it was still a fun little TS learning experience nonetheless. :)

So, why is there currently a 9 pipe limit, was that what you thought was a sensible limit, or is something specific causing the limitation? I am assuming that it is the first of those options considering that you said you can potentially extend it?

I hit the 9 pipe limit in a very similar way to @TeChn4K.

I have a v.object(...) as the schema in the pipe and then a lot of v.forward(v.check(...)); the reason for this is because of one attribute in the schema controlling whether everything else in the object is required or not.

Here is the minified schema:

const minLengthSchema = (length: number) => v.pipe(v.string(), v.minLength(length));

const schema = v.pipe(
  v.object({
    hasAlternativeContact: v.boolean(),
    firstName: v.pipe(
      v.string(),
      v.maxLength(200, validationMessages.maxChars(200))
    ),
    middleName: v.pipe(
      v.string(),
      v.maxLength(200, validationMessages.maxChars(200))
    ),
    lastName: v.pipe(
      v.string(),
      v.maxLength(200, validationMessages.maxChars(200))
    ),
    contactNo: v.pipe(
      v.string(),
      v.maxLength(20, validationMessages.maxChars(20))
    ),
    // ...with even more attributes with very similar schemas
  }),
  // Enforce that `firstName` has length if `hasAlternativeContact` is `true`
  v.forward(
    v.check(({ hasAlternativeContact, firstName } }) => {
      return hasAlternativeContact
        ? v.safeParse(minLengthSchema(1), firstName).success
        : true;
    }, validationMessages.requiredField),
    ['firstName']
  ),
  // ...with even more forward/check for all attributes in the object
)

This quickly caused me to go over the pipe limit, but I understand that there could be a better way of writing this schema, so open to suggestions on that, else the pipe being extended would rectify this issue.

@TeChn4K
Copy link

TeChn4K commented Jun 25, 2024

I would add that variant is not necessarily adapted to my schema, as there is a lot of branching and entanglement.

@fabian-hiller
Copy link
Owner

Thanks to both of you for your feedback! For simple data types, 9 is probably enough, but I did not think about more complex cases when I set the limit. We can extend the limit to any number. It just adds more TypeScript code. The JavaScript output remains the same. What limit do you think is appropriate? Is 19 enough or should we increase it to 29 or more?

@TeChn4K
Copy link

TeChn4K commented Jun 25, 2024

About twenty should be enough for me, but maybe not in the futur, I can't really known now. Too bad it can't be dynamic :/

Is there any bottleneck to do the following?

let baseSchema = v.object({
...
});

let pipedSchema1 = = v.pipe(baseSchema, v.forward(v.partialCheck(...), ['...']));
let pipedSchema2 = = v.pipe(pipedSchema1, v.forward(v.partialCheck(...), ['...']));
let pipedSchema3 = = v.pipe(pipedSchema2, v.forward(v.partialCheck(...), ['...']));
...
let pipedSchemaN = = v.pipe(pipedSchemaN-1, v.forward(v.partialCheck(...), ['...']));

@dyljhd
Copy link

dyljhd commented Jun 25, 2024

No problem.
Agreed, I have never got close to the 9 pipe limit for simple data types.
My example (if not minified) would have been 14 pipe items, so 19 pipe items would have been enough here.
Saying that, I feel like you could get to over 19 with a really complex data type in niche cases, so my gut is saying that 29 pipe items would be a more than sensible ceiling for 99% of use-cases. It could always be extended further in future if more people keep creating Github issues about this limit.

Yeah, would be nice to be dynamic @TeChn4K, but you would most likely run into a lot of TS hurdles, and if not implemented in a different way to what I suggested above, would cause a Typescript excessively deep warning pretty quickly - would be really cool if someone worked out a way to do it though if it is even possible - someone send this to Matt Pocock :').

@fabian-hiller
Copy link
Owner

Is there any bottleneck to do the following?

No, this should work fine. You can extend a pipe by nesting it as deep as you like.

My example (if not minified) would have been 14 pipe items, so 19 pipe items would have been enough here. Saying that, I feel like you could get to over 19 with a really complex data type in niche cases, so my gut is saying that 29 pipe items would be a more than sensible ceiling for 99% of use-cases. It could always be extended further in future if more people keep creating Github issues about this limit.

I will think about it and probably raise the limit to 19 or 29 in the next version.

As TypeScript improves its inference capabilities, it may be possible to dynamically type the pipe items in the future.

@fabian-hiller
Copy link
Owner

I have increased the limit in the latest version to 19

@dyljhd
Copy link

dyljhd commented Jun 28, 2024

@fabian-hiller
There are a few instances on the website where the pipe function is stated to have the ability to hold 9 pipe items.
I am assuming we want to update those as part of this change as well?
https://valibot.dev/guides/quick-start/
https://valibot.dev/guides/pipelines/

@fabian-hiller
Copy link
Owner

You are right! Thanks for the tip!

@fabian-hiller
Copy link
Owner

@stackoverfloweth I just saw that you downvoted my comment about increasing to 19 items. Feel free to give me feedback on that.

@stackoverfloweth
Copy link

@stackoverfloweth I just saw that you downvoted my comment about increasing to 19 items. Feel free to give me feedback on that.

It just that it circumvents the problem. I'm building composables on top of valibot that I'd like to take a pipe array arg for. I also really don't like the syntax of 19 overloads and would prefer not to have to reproduce that in my codebase to support pipe args.

@fabian-hiller
Copy link
Owner

I agree. I would do it differently if it were possible with TypeScript, but I couldn't find a workaround. Can you find one?

@dyljhd
Copy link

dyljhd commented Jul 31, 2024

Not sure if I have misunderstood the issue at hand here, but I'll mention my suggestion anyways.

There is an extreme problem on TypeHero that is all about working around the depth limit of TS, that from understanding, is the problem that is preventing there being an implicit definition of the pipe function without explicit overload definitions.

That could potentially ignite ideas on how to apply it to Valibot's types even if it isn't the direct solution.

Here is the problem: https://typehero.dev/challenge/inclusive-range
The submitted solutions are available for anyone to look at.

@fabian-hiller
Copy link
Owner

I think the depth limit is not the problem. I think the problem is that TS is not able to infer and limit the types correctly in our use case. We need to somehow make sure that the type of the next action matches the output of the previous action.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

No branches or pull requests

5 participants