Skip to content

Reduce the usage of lodash#505

Merged
valscion merged 9 commits intowebpack:masterfrom
SukkaW:drop-lodash
Aug 2, 2023
Merged

Reduce the usage of lodash#505
valscion merged 9 commits intowebpack:masterfrom
SukkaW:drop-lodash

Conversation

@SukkaW
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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?

valscion
valscion requested changes Aug 1, 2023
View reviewed changes
@valscion
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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:master Aug 2, 2023
@SukkaW
Copy link
Copy Markdown
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

+1 more reviewer

@tgmarinho tgmarinho tgmarinho approved these changes

Reviewers whose approvals may not affect merge requirements

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