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

bug: UnionValidator#setValidationEnabled does not make its members respect that #294

Open
1 task done
Qjuh opened this issue Aug 18, 2023 · 2 comments
Open
1 task done

Comments

@Qjuh
Copy link

Qjuh commented Aug 18, 2023

Is there an existing issue for this?

  • I have searched the existing issues

Description of the bug

Found when disabling validation in @discordjs/builders on EmbedBuilder#setDescription() it would still throw a CombinedError with ExpectedValidationError in it.

Steps To Reproduce

import {s} from "@sapphire/shapeshift";

s.string
.lengthGreaterThanOrEqual(10)
.nullish
.setValidationEnabled(false).parse("test");

Expected behavior

It should not throw any Error.

Screenshots

No response

Additional context

No response

@kyranet
Copy link
Member

kyranet commented Aug 19, 2023

Thing is, although the API is called setValidationEnabled, the getter method (getValidationEnabled()) is only called in tests, not a single time in the source code, and instead the library uses the getter below it (get shouldRunConstraints()):

public getValidationEnabled() {
return getValue(this.isValidationEnabled);
}
protected get shouldRunConstraints(): boolean {
return getValue(this.isValidationEnabled) ?? getGlobalValidationEnabled();
}

During validation, this.handle() is always called, even when get shouldRunConstraints() returns false:

public parse<R extends T = T>(value: unknown): R {
// If validation is disabled (at the validator or global level), we only run the `handle` method, which will do some basic checks
// (like that the input is a string for a string validator)
if (!this.shouldRunConstraints) {
return this.handle(value).unwrap() as R;
}
return this.constraints.reduce((v, constraint) => constraint.run(v).unwrap(), this.handle(value).unwrap()) as R;
}

In fact, if this.handle() wasn't called at all if it returned false, the effects would not only apply to the current validator, but also the children, since their parse() method wouldn't be called either.

We should change the name or the behaviour IMO. cc: @vladfrangu @favna

@favna
Copy link
Member

favna commented Aug 19, 2023

I'd say we should change the behaviour.

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

No branches or pull requests

3 participants