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

Prefer Object.assign to $.extend to merge objects #209

Merged
merged 3 commits into from
Mar 7, 2024

Conversation

guanw88
Copy link
Contributor

@guanw88 guanw88 commented Mar 6, 2024

Although Object.assign and $.extend have some differences (https://stackoverflow.com/questions/38345937/object-assign-vs-extend), the should not apply to us, as 1) we do not use the deep: true option in these methods, and 2) we are always merging in an empty object ({}), so it should never be undefined.

@guanw88 guanw88 changed the title Ref: Prefer Object.assign to $.extend to merge objects Prefer Object.assign to $.extend to merge objects Mar 6, 2024
@guanw88 guanw88 self-assigned this Mar 6, 2024
@guanw88 guanw88 requested a review from JustinRyanH March 7, 2024 02:01
Copy link
Member

@diclophis diclophis left a comment

Choose a reason for hiding this comment

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

this code appears to replace usage of $.extend with Object.assign correctly, however I would like to defer final approval to additional persons with more experience in these arts

Copy link
Member

@shirish-pampoorickal shirish-pampoorickal left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link

@jarednielson jarednielson left a comment

Choose a reason for hiding this comment

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

LGTM

@guanw88 guanw88 merged commit cf8af80 into master Mar 7, 2024
1 check passed
@guanw88 guanw88 deleted the start-removing-jquery branch March 7, 2024 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

5 participants