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

feat(utilities): Remove lodash dependency and replace it with custom functions #856

Merged
merged 22 commits into from
Aug 24, 2023
Merged
Show file tree
Hide file tree
Changes from 17 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
2 changes: 1 addition & 1 deletion .github/actions/audit-accessibility/main.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
// @ts-check
const path = require('path');
const os = require('os');
const _ = require('lodash');
const {execSync} = require('child_process');
const handler = require('serve-handler');
const http = require('http');
Expand Down Expand Up @@ -48,6 +47,7 @@ const startStorybook = () => {
const port = 6006;

const storybookServer = http.createServer((request, response) => {
// @ts-expect-error - type mismatch in response
return handler(request, response, {
public: 'public',
cleanUrls: ['/'],
Expand Down
4 changes: 2 additions & 2 deletions .github/actions/utils/azure-storage.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ const fs = require('fs');
const readFile = promisify(fs.readFile);
const core = require('@actions/core');

const ACCOUNT_NAME = core.getInput('azure-account-name') || process.env.INPUT_AZURE_ACCOUNT_NAME;
const ACCOUNT_KEY = core.getInput('azure-account-key') || process.env.INPUT_AZURE_ACCOUNT_KEY;
const ACCOUNT_NAME = core.getInput('azure-account-name') || process.env.INPUT_AZURE_ACCOUNT_NAME || '';
const ACCOUNT_KEY = core.getInput('azure-account-key') || process.env.INPUT_AZURE_ACCOUNT_KEY || '';

const CONTAINER_NAME = 'mistica-web-' + Date.now();

Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@
"jest-environment-puppeteer": "^6.1.1",
"jimp": "^0.16.1",
"lint-staged": "^12.3.7",
"lodash": "^4.17.21",
Copy link
Member Author

Choose a reason for hiding this comment

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

moved to devDependencies

"mini-css-extract-plugin": "^1.6.2",
"node-fetch": "^2.6.7",
"playroom": "^0.31.0",
Expand Down Expand Up @@ -149,7 +150,6 @@
"@vanilla-extract/dynamic": "^2.0.3",
"@vanilla-extract/sprinkles": "^1.5.1",
"classnames": "^2.3.1",
"lodash": "^4.17.21",
"moment": "^2.29.1",
"react-autosuggest": "^10.1.0",
"react-datetime": "^3.1.1",
Expand Down
8 changes: 8 additions & 0 deletions packages/generate-design-tokens/jsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"compilerOptions": {
"module": "CommonJS",
"target": "es2020",
"checkJs": true
},
"exclude": ["node_modules", ".yarn"]
}
2 changes: 1 addition & 1 deletion src/button-layout.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import {useIsomorphicLayoutEffect} from './hooks';
import {ButtonPrimary, ButtonSecondary, ButtonDanger} from './button';
import {BUTTON_MIN_WIDTH} from './button.css';
import classnames from 'classnames';
import debounce from 'lodash/debounce';
import {debounce} from './utils/lodash';
import {getPrefixedDataAttributes} from './utils/dom';
import * as styles from './button-layout.css';

Expand Down
2 changes: 1 addition & 1 deletion src/fixed-footer-layout.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import * as React from 'react';
import classnames from 'classnames';
import debounce from 'lodash/debounce';
import {debounce} from './utils/lodash';
import {isRunningAcceptanceTest} from './utils/platform';
import {
useElementDimensions,
Expand Down
2 changes: 1 addition & 1 deletion src/nestable-context.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import * as React from 'react';
import isEqual from 'lodash/isEqual';
import {isEqual} from './utils/lodash';

const useDeepCompareMemoize = (value: any) => {
const ref = React.useRef();
Expand Down
2 changes: 1 addition & 1 deletion src/switch-component.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ this storybook bug:
https://github.com/storybookjs/storybook/issues/11980
*/
import * as React from 'react';
import debounce from 'lodash/debounce';
import {debounce} from './utils/lodash';
import {SPACE} from './utils/key-codes';
import {useControlProps} from './form-context';
import {Text3} from './text';
Expand Down
115 changes: 115 additions & 0 deletions src/utils/__tests__/lodash-test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
import {debounce, isEqual} from '../lodash';

beforeEach(() => {
jest.useFakeTimers();
});

describe('debounce', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why describe?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is a way to organize the test suite. All debounce tests are placed inside the debounce describe section, same with isEqual or any other method.

According to jest doc:

describe() creates a block that groups together several related tests

And when you run the tests, they are shown with a pretty hierarchy
image

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't like it, and we don't use it anywhere. Organice the tests by file should be more than enough. And you already have a "debounce" or "isEqual" prefix in all your test names. It adds an extra nesting and can be tricky when combined with before/after methods

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't agree, but it also won't keep me awake.

Removed the describe blocks

test('debounce happy case', () => {
const fn = jest.fn().mockImplementation((a) => a);
const debounced = debounce(fn, 5000);

debounced(1);
jest.advanceTimersByTime(4500);
debounced(2);
jest.advanceTimersByTime(4500);
debounced(3);
expect(fn).not.toHaveBeenCalled();

jest.runAllTimers();

expect(fn).toHaveBeenCalledTimes(1);
expect(fn.mock.calls).toEqual([[3]]);
});

test('debounce with leading', () => {
const fn = jest.fn().mockImplementation((a) => a);
const debounced = debounce(fn, 5000, {leading: true});

debounced(1);
expect(fn.mock.calls).toEqual([[1]]);

debounced(2);
debounced(3);
expect(fn.mock.calls).toEqual([[1]]);

jest.runAllTimers();

expect(fn).toHaveBeenCalledTimes(2);
expect(fn.mock.calls).toEqual([[1], [3]]);
});

test('debounce with maxWait', () => {
const fn = jest.fn().mockImplementation((a) => a);
const debounced = debounce(fn, 2500, {maxWait: 3000});

debounced(1);
jest.advanceTimersByTime(2000);

debounced(2);
debounced(3);
jest.advanceTimersByTime(2000);
expect(fn.mock.calls).toEqual([[3]]);

debounced(4);
jest.runAllTimers();

expect(fn.mock.calls).toEqual([[3], [4]]);
});

test('debounce cancel', () => {
const fn = jest.fn().mockImplementation((a) => a);
const debounced = debounce(fn, 5000);

debounced(1);
debounced(2);
debounced(3);

debounced.cancel();

jest.runAllTimers();

expect(fn).not.toHaveBeenCalled();
});
});

describe('isEqual', () => {
const symbol = Symbol('abc');

test('isEqual happy case', () => {
const a = {
n: 123,
s: 'abc',
b: true,
nul: null,
und: undefined,
arr: [1, false, null, undefined, new Date(1234567890), {a: 1, b: 2, c: 3}],
date: new Date(1234567890),
obj: {a: 1, b: 2, c: 3},
symbol,
};

const b = {
n: 123,
s: 'abc',
b: true,
nul: null,
und: undefined,
arr: [1, false, null, undefined, new Date(1234567890), {a: 1, b: 2, c: 3}],
date: new Date(1234567890),
obj: {a: 1, b: 2, c: 3},
symbol,
};

expect(isEqual(a, b)).toBe(true);
});

test('isEqual with different primitives', () => {
expect(isEqual(1, 2)).toBe(false);
expect(isEqual('a', 'b')).toBe(false);
expect(isEqual(true, false)).toBe(false);
expect(isEqual(null, undefined)).toBe(false);
expect(isEqual(symbol, Symbol('abc'))).toBe(false);
expect(isEqual(new Date(1234567890), new Date(1234567891))).toBe(false);
});
});
120 changes: 120 additions & 0 deletions src/utils/lodash.tsx
pladaria marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
/**
* This file implements lodash alternatives used by components
*
* Importing the lodash library causes problems when building Mística as an ES module.
* Importing lodash-es causes problems when running ssr tests (not sure why)
*
* Once Mística gets migrated to a proper ES module, we can consider to remove this file and use lodash-es
*/

type Debounced<T> = T & {cancel: () => void};

export const debounce = <T extends (...args: Array<any>) => any>(
func: T,
wait: number,
options: {
leading?: boolean;
maxWait?: number;
} = {}
): Debounced<T> => {
let debounceTimeoutId: ReturnType<typeof setTimeout> | undefined;
let maxWaitTimeoutId: ReturnType<typeof setTimeout> | undefined;
let currentArgs: Parameters<T>;
let isLeading = true;

const debounced = (...args: Parameters<T>) => {
if (debounceTimeoutId) {
clearTimeout(debounceTimeoutId);
}

currentArgs = args;

if (!maxWaitTimeoutId && options.maxWait) {
maxWaitTimeoutId = setTimeout(() => {
func(...currentArgs);
maxWaitTimeoutId = undefined;
clearTimeout(debounceTimeoutId);
}, options.maxWait);
}

if (isLeading && options.leading) {
isLeading = false;
func(...args);
return;
}

debounceTimeoutId = setTimeout(() => {
func(...args);
if (maxWaitTimeoutId) {
clearTimeout(maxWaitTimeoutId);
}
debounceTimeoutId = undefined;
maxWaitTimeoutId = undefined;
// eslint-disable-next-line testing-library/await-async-utils
Copy link
Member Author

Choose a reason for hiding this comment

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

false positive. I've created an issue in js-toolbox to track these problems

https://github.com/Telefonica/js-toolbox/issues/new

}, wait);
};

debounced.cancel = () => {
if (debounceTimeoutId) {
clearTimeout(debounceTimeoutId);
debounceTimeoutId = undefined;
}
if (maxWaitTimeoutId) {
clearTimeout(maxWaitTimeoutId);
maxWaitTimeoutId = undefined;
}
};

return debounced as Debounced<T>;
};

const isPrimitive = (v: unknown): v is string | number | undefined | null | boolean | symbol => {
if (v === null) {
return true;
}
if (typeof v === 'object' || typeof v === 'function') {
return false;
}
return true;
};

export const isEqual = (a: unknown, b: unknown): boolean => {
if (a === b) {
return true;
}

if (isPrimitive(a) || isPrimitive(b)) {
return false;
}

if (typeof a !== typeof b) {
return false;
}

if (typeof a === 'function') {
// no need to check typeof b === 'function' because of the previous check
return false;
}

if (Array.isArray(a) || Array.isArray(b)) {
if (Array.isArray(a) && Array.isArray(b)) {
return a.length === b.length && a.every((value, index) => isEqual(value, b[index]));
}
return false;
}

if (a instanceof Date || b instanceof Date) {
if (a instanceof Date && b instanceof Date) {
return a.getTime() === b.getTime();
}
return false;
}

const keysA = Object.keys(a as any);
const keysB = Object.keys(b as any);
if (keysA.length === keysB.length) {
return keysA.every((key) => isEqual((a as any)[key], (b as any)[key]));
}

return false;
};
7 changes: 7 additions & 0 deletions vite.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,13 @@ export default defineConfig({
fileNames: ({name}) => `${name.replace(/\.css$/, '.css-mistica')}.js`,
}),
],
resolve: {
alias: {
// forbid lodash usage
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice trick

lodash: '/dev/null',
'lodash-es': '/dev/null',
},
},
publicDir: false,
build: {
lib: {
Expand Down
Loading