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

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

Merged
merged 1 commit into from
Jan 8, 2025
Merged

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

merged 1 commit into from
Jan 8, 2025

Conversation

Phillip9587
Copy link
Contributor

@Phillip9587 Phillip9587 commented Oct 28, 2024 •
edited
Loading

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

@Abdel-Monaam-Aouini
Copy link
Contributor

LGTM

LinusU
LinusU approved these changes Nov 4, 2024
View reviewed changes
Copy link
Member

@LinusU LinusU left a comment

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
Copy link
Contributor

@bhavya3024 bhavya3024 left a comment

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
lib/application.js Outdated Show resolved Hide resolved
lib/response.js Outdated Show resolved Hide resolved
test/res.cookie.js Outdated Show resolved Hide resolved
History.md Outdated Show resolved Hide resolved
wesleytodd
wesleytodd approved these changes Nov 13, 2024
View reviewed changes
Copy link
Member

@wesleytodd wesleytodd left a comment

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
Copy link
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!

@Phillip9587 Phillip9587 changed the title Remove utils-merge dependency - use Object.assign() instead Remove utils-merge dependency - use spread syntax instead Nov 13, 2024
@Phillip9587 Phillip9587 mentioned this pull request Dec 2, 2024
Closed
jonkoops
jonkoops reviewed Dec 2, 2024
View reviewed changes
lib/application.js Outdated Show resolved Hide resolved
@Phillip9587
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
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:master Jan 8, 2025
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bhavya3024 bhavya3024 bhavya3024 left review comments

@jonkoops jonkoops jonkoops left review comments

@bjohansebas bjohansebas bjohansebas approved these changes

@UlisesGascon UlisesGascon UlisesGascon approved these changes

@wesleytodd wesleytodd wesleytodd approved these changes

@LinusU LinusU LinusU 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.
8 participants
@Phillip9587 @Abdel-Monaam-Aouini @wesleytodd @LinusU @jonkoops @UlisesGascon @bhavya3024 @bjohansebas