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 #2397

Merged
merged 10 commits into from
Nov 12, 2024
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 packages/ui/components/form-core/src/FormControlMixin.js
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,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
13 changes: 9 additions & 4 deletions packages/ui/components/form-core/src/validate/ValidateMixin.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@ import { SyncUpdatableMixin } from '../utils/SyncUpdatableMixin.js';
import { LionValidationFeedback } from './LionValidationFeedback.js';
import { ResultValidator as MetaValidator } from './ResultValidator.js';
import { Unparseable } from './Unparseable.js';
import { Validator } from './Validator.js';
import { Required } from './validators/Required.js';
import { FormControlMixin } from '../FormControlMixin.js';

// eslint-disable-next-line no-unused-vars
import { Validator } from './Validator.js';
// TODO: [v1] make all @readOnly => @readonly and actually make sure those values cannot be set

/**
Expand Down Expand Up @@ -460,7 +460,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',
hardikpthv marked this conversation as resolved.
Show resolved Hide resolved
);
if (requiredValidator) {
this.__syncValidationResult = [{ validator: requiredValidator, outcome: true }];
}
Expand Down Expand Up @@ -685,7 +687,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
10 changes: 5 additions & 5 deletions packages/ui/scripts/cem.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,10 @@ for (const [, module] of moduleGraph.modules) {

/** Exclude information inherited from these mixins, they're generally not useful for public api */
const inheritanceDenyList = [
'LocalizeMixin',
'ScopedElementsMixin',
'SlotMixin',
'SyncUpdatableMixin',
'LocalizeMixin',
'SlotMixin',
];

const cem = create({
Expand All @@ -66,11 +66,11 @@ const cem = create({
* Set privacy for members based on naming conventions
*/
for (const m of declaration.members) {
if (!m.privacy && !m.name.startsWith('_') && !m.name.startsWith('#')) {
if (!m.privacy && !m.name?.startsWith('_') && !m.name?.startsWith('#')) {
m.privacy = 'public';
} else if (!m.privacy && m.name.startsWith('_')) {
} else if (!m.privacy && m.name?.startsWith('_')) {
m.privacy = 'protected';
} else if (m.name.startsWith('#') || m.name.startsWith('__')) {
} else if (m.name?.startsWith('#') || m.name?.startsWith('__')) {
m.privacy = 'private';
}
}
Expand Down