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(validate-mixin): determine required or result validators based on characteristics #2498

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

meteabduraman
Copy link

Problem

In ValidateMixin.js a combination of approaches was used to determine if a Required validator is a Required validator, so that the validation routines can be executed accordingly. This involved:

However, an instanceof check still remained in the __executeValidators function. This PR aims to unify the approach in ValidateMixin.js.

What I did

  1. When classifying the passed validators (in MetaValidators, AsyncValidators, SyncValidators), both the Required check and the MetaValidator checks are now done via characteristics of each:

    • the Required validator should have .validatorName === 'Required'
    • the MetaValidator should present a .executeOnResults function
  2. Improved some JSDOC type annotations

  3. Added 2 new test cases in ValidateMixin.suite.js:

    • registering a Required validator which is bundled (as an example) should keep working, while with an instanceof check, the test would fail in this case
    • same for a ResultValidator (in this case DefaultSuccess)

Copy link

changeset-bot bot commented Mar 21, 2025

🦋 Changeset detected

Latest commit: a830c83

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@lion/ui Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@CLAassistant
Copy link

CLAassistant commented Mar 21, 2025

CLA assistant check
All committers have signed the CLA.

@@ -130,6 +130,121 @@ export function runValidateMixinSuite(customConfig) {
expect(el.hasFeedbackFor).to.deep.equal(['error']);
});

it('determines whether the "Required" validator was already handled by judging the validatorName', async () => {
class BundledValidator extends EventTarget {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you maybe extend from the Validator class here (to set the right example in tesrs as well?)

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

Successfully merging this pull request may close these issues.

3 participants