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

Skip pipeline when calling is() #166

Closed
wants to merge 2 commits into from

Conversation

jonlambert
Copy link
Contributor

As is() is primarily designed for use as a type guard, it would make sense to skip pipelines during this check too 🙂

Currently:

is(string([email()]), 'foo') // this check would fail, though types are correct

Proposed:

is(string([email()]), 'foo') // this check would pass, as it's a string

Notes

This is a breaking change to is()

Following on from #164.

@netlify
Copy link

netlify bot commented Sep 17, 2023

Deploy Preview for valibot ready!

Name Link
🔨 Latest commit 597af57
🔍 Latest deploy log https://app.netlify.com/sites/valibot/deploys/6508808d9ab0d0000805fe52
😎 Deploy Preview https://deploy-preview-166--valibot.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@fabian-hiller
Copy link
Owner

fabian-hiller commented Sep 17, 2023

To just check the data type, you are right and it can be added. However, I would add it as a configuration and leave it as it is by default. I strongly suspect that this is the expected behavior of most Valibot users.

// Default behaviour with pipes
is(Schema, input);

// Skip pipes and only check data type
is(Schema, input, { skipPipe: true });

// Or just with an boolean
is(Schema, input, true);

@fabian-hiller fabian-hiller self-assigned this Sep 17, 2023
@fabian-hiller fabian-hiller added the enhancement New feature or request label Sep 17, 2023
@jonlambert
Copy link
Contributor Author

Yep fair enough! I guess it depends on the primary purpose of is(). Personally I'd use it as a typecheck, so would prefer the simple boolean, but I defer to your judgement there, @fabian-hiller 🙂 Thanks again for your consideration!

@fabian-hiller
Copy link
Owner

If you are interested in this feature, you can implement it. Otherwise we can wait for feedback from other people.

@jonlambert
Copy link
Contributor Author

jonlambert commented Sep 18, 2023

Yep happy to implement, @fabian-hiller 🙂 I do think it might be worth waiting for some more opinions though, if that's ok. I'm undecided about what might be the best option.

To add to the options you've listed above:

// Might be more clearly represented with an opt-out approach
is(Schema, input, false);

// Function signature would be
export function is<TSchema extends BaseSchema>(
  schema: TSchema,
  input: unknown,
  runPipe = true
): input is Input<TSchema> {
  return !schema._parse(input, { abortEarly: true, skipPipe: !runPipe }).issues;
}

Would be good to hear from others what they might expect the behaviour of is() to be, as in the docs it's very much presented as a type guard 🙂

@fabian-hiller
Copy link
Owner

I think that if someone defines the schema string([email()]) and then uses is, they expect false to be returned for an empty string. If we only validated string and not email, I suspect this would lead to bugs in many cases.

Maybe we should implement it explicitly with is(schema, input, { skipPipe: true }) for now, leaving the possibility to add more configurations in the future. Furthermore this way it is consistent with parse and safeParse.

@jonlambert
Copy link
Contributor Author

jonlambert commented Sep 18, 2023

Yeah I do agree with you, @fabian-hiller. Definitely good to keep consistent with parse and safeParse. I think you're right that it would be unexpected for it to ignore checks invisibly.

I've added the info argument to is() 🙂 I've kept abortEarly: true by default to ensure we retain current behaviour.

@jasikpark
Copy link

I think that if someone defines the schema string([email()]) and then uses is, they expect false to be returned for an empty string. If we only validated string and not email, I suspect this would lead to bugs in many cases.

Maybe we should implement it explicitly with is(schema, input, { skipPipe: true }) for now, leaving the possibility to add more configurations in the future. Furthermore this way it is consistent with parse and safeParse.

I would agree, I would expect a schema passed to is() to fully check the schema shape - if we were clever email() could further refine the shape of the string 🤷

@fabian-hiller
Copy link
Owner

I added skipPipe in v0.22.0 as a configuration to is.

// Skip pipes and only check data type
is(Schema, input, { skipPipe: true });

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

Successfully merging this pull request may close these issues.

None yet

3 participants