Skip to content

Remove utils-merge dependency - use spread syntax instead - #6091

Merged
wesleytodd merged 1 commit into
expressjs:masterfrom
Phillip9587:remove-utils-merge
Jan 8, 2025
Merged

Remove utils-merge dependency - use spread syntax instead#6091
wesleytodd merged 1 commit into
expressjs:masterfrom
Phillip9587:remove-utils-merge

Conversation

@Phillip9587

@Phillip9587Phillip9587 commented Oct 28, 2024•
edited
Loading

Copy link
Copy Markdown
Member

Removes the utils-merge dependency and uses Object.assign() spread syntax

@Abdel-Monaam-Aouini

Abdel-Monaam-Aouini commented Nov 3, 2024

Copy link
Copy Markdown
Contributor

LGTM

LinusU
LinusU approved these changes Nov 4, 2024
View reviewed changes

@LinusULinusU left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

From reading the utils-merge source code, it seems like this should be functionally equivalent. Neat 👍

bhavya3024
bhavya3024 reviewed Nov 13, 2024
View reviewed changes

@bhavya3024bhavya3024 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅

wesleytodd
wesleytodd requested changes Nov 13, 2024
View reviewed changes
Comment threadlib/application.js Outdated
Comment threadlib/response.js Outdated
Comment threadtest/res.cookie.js Outdated
Comment threadHistory.md Outdated
wesleytodd
wesleytodd approved these changes Nov 13, 2024
View reviewed changes

@wesleytoddwesleytodd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We will need to revisit this when we get benchmarking fully setup, but I think this is a pretty nice and easy perf win for render.

@wesleytodd

wesleytodd commented Nov 13, 2024

Copy link
Copy Markdown
Member

We can let this sit after the new changes for a bit to give folks some time to weight in, but awesome work @Phillip9587!

@Phillip9587Phillip9587 changed the title Remove utils-merge dependency - use Object.assign() insteadRemove utils-merge dependency - use spread syntax insteadNov 13, 2024
@Phillip9587Phillip9587 mentioned this pull request Dec 2, 2024
Closed
jonkoops
jonkoops reviewed Dec 2, 2024
View reviewed changes
Comment threadlib/application.js Outdated
@Phillip9587

Phillip9587 commented Dec 17, 2024

Copy link
Copy Markdown
MemberAuthor

Hey @wesleytodd! Just wanted to check in and see if there’s anything I can help with to move this PR forward. Let me know if I can make any changes or provide additional info. Thanks!

@Phillip9587

Phillip9587 commented Jan 8, 2025

Copy link
Copy Markdown
MemberAuthor

Hi @wesleytodd@UlisesGascon! Apologies for the additional ping. I hope you're both doing well. I noticed this PR has received three approvals and no objections. Please let me know if there’s anything else needed from my side. Thank you so much!

@wesleytodd

wesleytodd commented Jan 8, 2025

Copy link
Copy Markdown
Member

Hey, nothing to do on your side. I am catching up (and especially over the holidays a lot of folks go ooo) so will be working across all of these soon. This one looks good to merge though, so doing so now. Will be looking to prepare releases for all this in the coming weeks.

@wesleytodd
wesleytodd merged commit 246f6f5 into expressjs:masterJan 8, 2025
@bjohansebasbjohansebas mentioned this pull request Mar 15, 2025
Closed
@UlisesGasconUlisesGascon mentioned this pull request Mar 23, 2025
Closed
68 tasks
@wesleytoddwesleytodd mentioned this pull request Mar 28, 2025
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@LinusULinusULinusU approved these changes
@wesleytoddwesleytoddwesleytodd approved these changes
@UlisesGasconUlisesGasconUlisesGascon approved these changes
@bjohansebasbjohansebasbjohansebas approved these changes
+2 more reviewers
@jonkoopsjonkoopsjonkoops left review comments
@bhavya3024bhavya3024bhavya3024 left review comments
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.

8 participants

@Phillip9587@Abdel-Monaam-Aouini@wesleytodd@LinusU@jonkoops@UlisesGascon@bhavya3024@bjohansebas