Skip to content
Open
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
5 changes: 4 additions & 1 deletion packages/components/src/types/property-types.ts
Original file line number Diff line number Diff line change
@@ -1 +1,4 @@
export type PropertyType = 'boolean' | 'number' | 'string' | 'array' | 'object' | 'function';
export type PrimitiveType = 'boolean' | 'number' | 'string';
export type ReferenceType = 'array' | 'object' | 'function';

export type PropertyType = PrimitiveType | ReferenceType;
3 changes: 3 additions & 0 deletions packages/components/src/utils/clamp.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export function clamp(value: number, min: number, max: number): number {
return Math.min(Math.max(value, min), max);
}
4 changes: 1 addition & 3 deletions packages/components/src/utils/is-value-empty.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
import { EMPTY_VALUES } from './property-checkers/constants';

export function isValueEmpty(value: unknown): boolean {
return EMPTY_VALUES.some(v => v === value);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest to keep the รฌsValueEmpty` function as is but change the way we compare the values.

The following would allow us to compare NaN with NaN, beside all the other values:
return EMPTY_VALUES.some(v => Object.is(v, value));

Test:

  • NaN === NaN -> false
  • Object.is(NaN, NaN) -> true

The idea behind this is, to allow editing the tested values โ€‹โ€‹without touching the test logic itself, thus minimizing the risk of regressions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We didnโ€™t notice the function wasnโ€™t working earlier because both the function and its tests were using the same EMPTY_VALUES array. As a result, NaN === NaN evaluated to true, but only because both values referred to the exact same NaN instance from that shared array. In general, itโ€™s best to decouple tests from implementation details as much as possible to avoid false negatives like this.

return value == null || value === '' || (typeof value === 'number' && isNaN(value));
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import { PrimitiveType } from '@/types/property-types';

export function checkArrayOf<T extends { host: HTMLElement }>(
component: T,
prop: keyof T,
type: PrimitiveType,
) {
const componentName = component.host.localName;
const value = component[prop];

const message = `The prop \`${String(
prop,
)}\` of the \`${componentName}\` component must be an \`${type}\` array.`;

if (!Array.isArray(value) || value.some(val => typeof val !== type)) {
console.error(message);
}
}
3 changes: 3 additions & 0 deletions packages/components/src/utils/property-checkers/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,16 @@ import { checkPattern } from './check-pattern';
import { checkType } from './check-type';
import { checkUrl } from './check-url';
import { emptyOr } from './empty-or';
import { checkArrayOf } from '@/utils/property-checkers/check-array-of';

export const checkEmptyOrOneOf = emptyOr(checkOneOf);
export const checkEmptyOrPattern = emptyOr(checkPattern);
export const checkEmptyOrType = emptyOr(checkType);
export const checkEmptyOrUrl = emptyOr(checkUrl);
export const checkEmptyOrArrayOf = emptyOr(checkArrayOf);

export const checkRequiredAndOneOf = requiredAnd(checkOneOf);
export const checkRequiredAndPattern = requiredAnd(checkPattern);
export const checkRequiredAndType = requiredAnd(checkType);
export const checkRequiredAndUrl = requiredAnd(checkUrl);
export const checkRequiredAndArrayOf = requiredAnd(checkArrayOf);
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
import { checkArrayOf } from '../check-array-of';
import { PrimitiveType } from '@/types';
import { describe } from 'node:test';

describe('checkArrayOf', () => {
const componentName = 'post-component';
const propName = 'myProp';
const mockValues = [
undefined,
null,
true,
false,
42,
NaN,
'string',
'',
[],
{},
() => {
/* empty */
},
];

let consoleErrorSpy: jest.SpyInstance;

beforeEach(() => {
consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation(() => {});
});

afterEach(() => {
consoleErrorSpy.mockRestore();
});

const primitiveTypes: PrimitiveType[] = ['boolean', 'string', 'number'];
primitiveTypes.forEach(type => {
describe(type, () => {
const error = `The prop \`${propName}\` of the \`${componentName}\` component must be an \`${type}\` array.`;

const runCheckForValue = (value: unknown) => {
const component = { host: { localName: componentName } as HTMLElement, [propName]: value };
checkArrayOf(component, propName, type);
};

it('should log an error if the value is not an array', () => {
mockValues
.filter(value => !Array.isArray(value))
.forEach(value => {
runCheckForValue(value);
expect(consoleErrorSpy).toHaveBeenCalledWith(expect.stringContaining(error));
});
});

it('should log an error if the array contains some values that don\'t have the expected type', () => {
runCheckForValue(mockValues);
expect(consoleErrorSpy).toHaveBeenCalledWith(expect.stringContaining(error));
});

it('should not log an error if the array contains only values with the expected type', () => {
const validArray = mockValues.filter(value => typeof value === type);
runCheckForValue(validArray);
expect(consoleErrorSpy).not.toHaveBeenCalled();
});
});
});
});
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
import { requiredAnd } from '../required-and';
import { EMPTY_VALUES } from '../constants';
describe('requiredAnd', () => {
const mockCheck = jest.fn();

const mockRequiredAndCheck = requiredAnd(mockCheck);

it('should throw error if the provided value is empty', () => {
EMPTY_VALUES.forEach(emptyValue => {
[undefined, null, '', NaN].forEach(emptyValue => {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we have to update the values which are considered as "empty", we also have to update the test.
If on the other side we can keep the constant EMPTY_VALUES on both side, this is not the case and the maintenance effort can be kept at a minimum.

const component = { host: { localName: 'post-component' } as HTMLElement, prop: emptyValue };
const prop = component['prop'];
const error = `The prop \`${emptyValue}\` of the \`post-component\` component is not defined.`;
Expand All @@ -17,7 +16,6 @@ describe('requiredAnd', () => {
it('should run the check if the provided value is not empty', () => {
[
0,
NaN,
' ',
false,
[],
Expand All @@ -36,7 +34,7 @@ describe('requiredAnd', () => {
});

it('should pass all provided arguments to the nested check function', () => {
const args = ['non empty value', true, false, ['arg in an array'], { arg: 'in an object' }];
const args = [0, false, 'text', [], {}];

args.forEach(arg => {
const component = { host: { localName: 'post-component' } as HTMLElement, prop: arg };
Expand Down
15 changes: 15 additions & 0 deletions packages/components/src/utils/tests/clamp.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import { clamp } from '../clamp';

describe('clamp', () => {
it('should return the provided value if within the bounds', () => {
expect(clamp(10, 0, 100)).toEqual(10);
});

it('should return the min if provided value is lower', () => {
expect(clamp(0, 10, 100)).toEqual(10);
});

it('should return the max if provided value is greater', () => {
expect(clamp(100, 0, 10)).toEqual(10);
});
});
Loading