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: form validators check and disabled prop #2405

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/chilled-games-care.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'@lion/ui': patch
---

Add unique ['_$isValidator$'] marker to identify Validator instances across different lion versions.
It's meant to be used as a replacement for an `instanceof` check.
2 changes: 1 addition & 1 deletion package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion packages/ui/components/form-core/src/FormControlMixin.js
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,7 @@ const FormControlMixinImplementation = superclass =>
super.updated(changedProperties);

if (changedProperties.has('disabled')) {
this._inputNode?.setAttribute('aria-disabled', this.disabled.toString());
this._inputNode?.setAttribute('aria-disabled', `${Boolean(this.disabled)}`);
}

if (changedProperties.has('_ariaLabelledNodes')) {
Expand Down
10 changes: 8 additions & 2 deletions packages/ui/components/form-core/src/validate/ValidateMixin.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { SyncUpdatableMixin } from '../utils/SyncUpdatableMixin.js';
import { LionValidationFeedback } from './LionValidationFeedback.js';
import { ResultValidator as MetaValidator } from './ResultValidator.js';
import { Unparseable } from './Unparseable.js';
// eslint-disable-next-line no-unused-vars
import { Validator } from './Validator.js';
import { Required } from './validators/Required.js';
import { FormControlMixin } from '../FormControlMixin.js';
Expand Down Expand Up @@ -460,7 +461,9 @@ export const ValidateMixinImplementation = superclass =>

if (isEmpty) {
const hasSingleValue = !(/** @type {* & ValidateHost} */ (this)._isFormOrFieldset);
const requiredValidator = this._allValidators.find(v => v instanceof Required);
const requiredValidator = this._allValidators.find(
v => /** @type {typeof Validator} */ (v.constructor)?.validatorName === 'Required',
);
if (requiredValidator) {
this.__syncValidationResult = [{ validator: requiredValidator, outcome: true }];
}
Expand Down Expand Up @@ -685,7 +688,10 @@ export const ValidateMixinImplementation = superclass =>
}

for (const validatorToSetup of this._allValidators) {
if (!(validatorToSetup instanceof Validator)) {
// disable dot notation to avoid the renaming for the prop during build/minification
const validatorCtor = /** @type {typeof Validator} */ (validatorToSetup.constructor);
// eslint-disable-next-line dot-notation
if (validatorCtor['_$isValidator$'] === undefined) {
// throws in constructor are not visible to end user so we do both
const errorType = Array.isArray(validatorToSetup) ? 'array' : typeof validatorToSetup;
const errorMessage = `Validators array only accepts class instances of Validator. Type "${errorType}" found. This may be caused by having multiple installations of "@lion/ui/form-core.js".`;
Expand Down
20 changes: 17 additions & 3 deletions packages/ui/components/form-core/src/validate/Validator.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
/**
* @typedef {import('../../types/validate/index.js').FeedbackMessageData} FeedbackMessageData
* @typedef {import('../../types/validate/index.js').ValidatorParam} ValidatorParam
* @typedef {import('../../types/validate/index.js').ValidatorConfig} ValidatorConfig
* @typedef {import('../../types/validate/index.js').ValidatorOutcome} ValidatorOutcome
* @typedef {import('../../types/validate/index.js').ValidatorName} ValidatorName
* @typedef {import('../../types/validate/index.js').ValidatorConfig} ValidatorConfig
* @typedef {import('../../types/validate/index.js').ValidatorParam} ValidatorParam
* @typedef {import('../../types/validate/index.js').ValidationType} ValidationType
* @typedef {import('../../types/validate/index.js').ValidatorName} ValidatorName
* @typedef {import('../FormControlMixin.js').FormControlHost} FormControlHost
*/

Expand Down Expand Up @@ -32,8 +32,22 @@ export class Validator extends EventTarget {
* @type {ValidationType}
*/
this.type = config?.type || 'error';
/**
* Disable dot notation to avoid the renaming for the prop during build/minification
*/
}

/**
* This unique marker allows us to identify Validator instances across different lion versions.
* It's meant to be used as a replacement for an instanceof check.
*
* N.B. it's important to keep the bracket notation, as `static _$isValidator$ = true;` will
* be renamed during minification and that leads to different names across different
* lion versions.
* @type {boolean}
*/
static ['_$isValidator$'] = true;

/**
* The name under which validation results get registered. For convenience and predictability, this
* should always be the same as the constructor name (since it will be obfuscated in js builds,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
/* eslint-disable dot-notation */
import { LitElement } from 'lit';
import { defineCE, expect, fixture, html, unsafeStatic } from '@open-wc/testing';
import sinon from 'sinon';
Expand Down Expand Up @@ -187,6 +188,13 @@ describe('Validator', () => {
expect(disconnectSpy.calledWith(el)).to.equal(true);
});

it('adds static ["_$isValidator$"] property as a marker to identify the Validator class across different lion versions (as instanceof cannot be used)', async () => {
const myValidator = new Validator();

expect(myValidator.constructor['_$isValidator$']).to.exist;
expect(myValidator.constructor['_$isValidator$']).to.be.true;
});

describe('Types', () => {
it('has type "error" by default', async () => {
expect(new Validator().type).to.equal('error');
Expand Down
Loading