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

fix fallbackAsync for async arg & add relevant tests #732

Merged

Conversation

EltonLobo07
Copy link
Contributor

This PR fixes the issue related to fallbackAsync given any async schema.

Issue:

When the validation of an async schema fails, fallbackAsync still returns the default value instead of the supplied fallback value. The code block given below exposes this bug.

import * as v from 'valibot';

const Schema = v.fallbackAsync(
  v.pipeAsync(
    v.string(),
    v.checkAsync(async (value) => false)
  ),
  "fallback"
);

// logs: default
console.log(await v.safeParseAsync(Schema, "default"));

Fix:

Inside the _run method of the object returned by fallbackAsync, wait for the schema to run completely before checking if the dataset has any issues. This is needed as fallbackAsync can be given an async schema that might execute some async code.

Additional information:

Simply waiting for the async code to complete fixes the issue, but I was worried about the rejected promise case. I checked some of the other methods and found out that there is no logic to catch rejected promises. I assume this is intentional, and the thought process behind this approach would be to let the user handle rejected promises. Let me know if my assumption is incorrect.

@fabian-hiller
Copy link
Owner

Thanks for this fix! Yes, at the moment we expect the user to catch async errors.

@fabian-hiller fabian-hiller self-assigned this Jul 21, 2024
@fabian-hiller fabian-hiller added the fix A smaller enhancement or bug fix label Jul 21, 2024
@fabian-hiller fabian-hiller merged commit 7908989 into fabian-hiller:main Jul 21, 2024
6 checks passed
@fabian-hiller
Copy link
Owner

v0.37.0 is available

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix A smaller enhancement or bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants