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

rawCheck accepts a function that returns a Promise #832

Open
EltonLobo07 opened this issue Sep 14, 2024 · 4 comments
Open

rawCheck accepts a function that returns a Promise #832

EltonLobo07 opened this issue Sep 14, 2024 · 4 comments
Assignees
Labels
question Further information is requested

Comments

@EltonLobo07
Copy link
Contributor

The code block shown below should generate a type error as rawCheck might not be designed to use async functions.

import * as v from 'valibot';

const Schema = v.pipe(
  v.object({key: v.string()}),
  // This should generate a type error
  // as the function returns a `Promise`
  v.rawCheck(async () => {})
);
@EltonLobo07
Copy link
Contributor Author

This is because the return type Promise<void> is accepted by void. More information here.

@fabian-hiller
Copy link
Owner

Thanks for the hint! I am not sure what to do. void seems to be the correct type, but it is true that async functions result in run type errors. Do you have a recommendation?

@fabian-hiller fabian-hiller self-assigned this Sep 14, 2024
@fabian-hiller fabian-hiller added the question Further information is requested label Sep 14, 2024
@EltonLobo07
Copy link
Contributor Author

I found two ideas that might work:

  1. Create a loose union that accepts anything except a promise like object - details.
  2. Use a union of undefined and void as the return type - details.

I don't think the first idea is enough to handle all of the cases. So let's forget it.

The second idea might work. I'll talk about it below.
Let's assume we are working with a function type () => string. Usually, it is assignable to () => void as explained here. But () => string is not assignable to () => undefined | void. I assume this is because of the compiler using a different comparison for void union types.
Why exactly do we need a union of undefined and void?

function rawCheck(callback: () => (undefined | void)) {

}

undefined for these cases:

rawCheck(() => { }); // () => undefined
rawCheck(() => { return; }); // () => undefined
rawCheck(() => undefined); // () => undefined

void for this case:

const voidFn: () => void = () => 12;
rawCheck(voidFn); // () => void

These cases are flagged as errors:

rawCheck(() => null); // () => null
rawCheck((() => 12)); // () => 12
rawCheck(async () => { }); // () => Promise<null>

I feel this is a reasonably ask as () => string can be easily converted to () => undefined using a function wrapper.
But even then, I don't think this idea will prevent users from passing functions that return promises. Here's an example that takes us back to where we started:

const anotherVoidFn: () => void = async () => 12;
rawCheck(anotherVoidFn);

Even though the example is simple, I think large codebases can easily be in a similar situation.

I would recommend using () => undefined. Why?

  1. TS 5.1 made working with this type of function good- details.
  2. For functions that don't return undefined but void, we can recommend using a function wrapper to convert the function (something like: () => { fnReturningVoid(); }). On top of this, some users might even see a warning or an error by the linter related to floating promises (if fnReturningVoid returned Promise<anyTypeHere>).

What do you think?

@fabian-hiller
Copy link
Owner

I think I don't want to make a decision right now, as other things are much more important and void seems more technically correct. I will probably have another look at this issue when we are in v1 RC and investigate the undefined solution. Thank you for your research!

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

No branches or pull requests

2 participants