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.

Sign up for GitHub

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

Jump to bottom

Reduce the usage of lodash #505

Merged
merged 9 commits into from
Aug 2, 2023
Merged

Reduce the usage of lodash #505

merged 9 commits into from
Aug 2, 2023

Conversation

SukkaW
Copy link
Contributor

@SukkaW SukkaW commented Mar 31, 2022

Why do we need lodash if we can utilize features provided by the ECMAScript itself :)

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Mar 31, 2022 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

valscion
valscion reviewed Jul 31, 2023
View reviewed changes
Copy link
Member

@valscion valscion left a comment

Choose a reason for hiding this comment

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

Yeah seems like these changes end up having the same outcome as the lodash code. Can you address my review comment and also do a changelog entry with an "Internal" tag?

src/tree/utils.js Show resolved Hide resolved
valscion
valscion requested changes Aug 1, 2023
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@valscion
Copy link
Member

valscion commented Aug 1, 2023

Looks like some tests are breaking. Have you tried if running tests locally pass for you?

@SukkaW
Copy link
Contributor Author

SukkaW commented Aug 1, 2023 •
edited
Loading

Looks like some tests are breaking. Have you tried if running tests locally pass for you?

Yeah, I have been looking into this. However, it only shows timeout (instead of test mismatch) on my side.

Update: @valscion I have fixed all failed test cases found on my machine.

@SukkaW SukkaW requested a review from valscion August 1, 2023 15:03
@valscion
Copy link
Member

valscion commented Aug 2, 2023

Looks like there are still some test failures only found in this PR and also lint errors.

@SukkaW
Copy link
Contributor Author

SukkaW commented Aug 2, 2023

Looks like there are still some test failures only found in this PR and also lint errors.

Fixed the viewer.js test in f162c20 (#505) and fix lint errors from npm run lint in ead9a9b (#505).

valscion
valscion approved these changes Aug 2, 2023
View reviewed changes
Copy link
Member

@valscion valscion left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me! The diff is small enough that it seems like it shouldn't cause any regressions.

This doesn't look like it needs to be urgently released so I'll give it some time until there are other things that warrant a new release ☺️

@valscion valscion merged commit e120231 into webpack-contrib:master Aug 2, 2023
5 checks passed
@SukkaW
Copy link
Contributor Author

SukkaW commented Aug 2, 2023

Thanks, looks good to me! The diff is small enough that it seems like it shouldn't cause any regressions.

This doesn't look like it needs to be urgently released so I'll give it some time until there are other things that warrant a new release ☺️

And I am planning more PRs (that I want to be included in the next release) as well.

@SukkaW SukkaW mentioned this pull request Aug 2, 2023
Merged
@SukkaW SukkaW mentioned this pull request Apr 7, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@valscion valscion valscion approved these changes

@tgmarinho tgmarinho tgmarinho approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@SukkaW @valscion @tgmarinho