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

Conversation

pladaria
Copy link
Member

@pladaria pladaria commented Aug 18, 2023

This will help us to make mistica a fully ES Module library. In my tests, when changing the package type to module, the build was complaining about lodash (which is a cjs library).

I tried to replace it with lodash-es but for some reason that I was unable to understand, that produced problems with the SSR acceptance tests, so at the end we decided to fully remove lodash from mistica components (and we saved 2.2 kB)

@github-actions
Copy link

github-actions bot commented Aug 18, 2023

Size stats

master this branch diff
Total JS 9.45 MB 9.45 MB +1.5 kB
JS without icons 951 kB 952 kB +1.5 kB
Lib overhead 128 kB 125 kB -2.18 kB
Lib overhead (gzip) 33 kB 32.2 kB -799 B

@github-actions
Copy link

github-actions bot commented Aug 18, 2023

Deploy preview for mistica-web ready!

✅ Preview
https://mistica-4mjvkgfq7-tuentisre.vercel.app

Built with commit 18406da.
This pull request is being automatically deployed with vercel-action

@github-actions
Copy link

github-actions bot commented Aug 18, 2023

Accessibility report
✔️ No issues found

ℹ️ You can run this locally by executing yarn audit-accessibility.

@pladaria pladaria marked this pull request as draft August 21, 2023 15:23
@pladaria pladaria changed the title WEB-1468 use lodash-es instead of lodash fix(Build): Remove lodash dependency Aug 21, 2023
@@ -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

src/utils/lodash.tsx Outdated Show resolved Hide resolved
@pladaria pladaria changed the title fix(Build): Remove lodash dependency feat(utilities): Remove lodash dependency and replace it with custom functions Aug 22, 2023
@pladaria pladaria marked this pull request as ready for review August 22, 2023 07:57
src/utils/lodash.tsx Outdated Show resolved Hide resolved
}
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

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

src/utils/lodash.tsx Outdated Show resolved Hide resolved
@@ -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

@pladaria pladaria requested a review from atabel August 23, 2023 09:36
@pladaria pladaria added this pull request to the merge queue Aug 24, 2023
Merged via the queue into master with commit dcaf691 Aug 24, 2023
10 checks passed
@pladaria pladaria deleted the pladaria/WEB-1468_use-lodash-es branch August 24, 2023 09:40
tuentisre pushed a commit that referenced this pull request Aug 24, 2023
# [14.21.0](v14.20.1...v14.21.0) (2023-08-24)

### Bug Fixes

* **Cards:** remove min width from all the cards ([#858](#858)) ([029a6ea](029a6ea))
* **Tabs:** remove extra margin in safari ([#857](#857)) ([be8c2f2](be8c2f2))

### Features

* **FeedbackScreens:** updates in icons, paddings and styling ([#852](#852)) ([4b231e6](4b231e6))
* **Sheet:** new component ([#840](#840)) ([f03ca7c](f03ca7c))
* **utilities:** Remove lodash dependency and replace it with custom functions ([#856](#856)) ([dcaf691](dcaf691))
@tuentisre
Copy link
Collaborator

🎉 This PR is included in version 14.21.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants