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

replace broken utils.extend functionality with object.assign #1055

Merged
merged 2 commits into from
Mar 21, 2017

Conversation

snapwich
Copy link
Collaborator

Type of change

  • Bugfix

Description of change

The utils#extend functionality was erroneously converting array properties in the source object to plain objects in the target object. I found this bug when attempting to loop over the bidderRequest.bids array in a BID_REQUESTED event handler and saw that events.getEvents() was converting the bids array into a plain object.

Since Object.assign is available in Babel I removed utils.extend and replaced with Object.assign in every place I could find. Object.assign also has the added benefit of allowing multiple sources.

@@ -44,7 +47,7 @@ describe ('LifestreetAdapter', () => {

beforeEach(() => {
tagRequests = [];
request = utils.extend(request, BIDDER_REQUEST);
request = copy(BIDDER_REQUEST);
Copy link
Member

Choose a reason for hiding this comment

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

why copy here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

for whatever reason, just replacing utils.extend with Object.assign was causing the tests to fail. I think they were built in away that made them dependent on the broken utils.extend functionality. I didn't do a deep-dive to figure out why they were failing, but I noticed that the intent was to have a fresh request object before each unit test; copy worked without breaking the tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also, Object.assign({}, request, BIDDER_REQUEST) did not work either.

@snapwich
Copy link
Collaborator Author

@mkendall07 Coincidentally a co-worker's build was breaking today because the AOL adapter spec was requiring lodash for cloneDeep. lodash isn't listed as a direct dependency in the package.json but is an indirect dependency of some other library. This was causing the build to break on npm v2, but not v3 (which flattens the dependency tree and does dependency resolution).

I just updated this pull-request and added that copy function as a utility function, utils.cloneDeep , and updated both the calls in the lifestreet_spec and aol_spec to use it.

@mkendall07
Copy link
Member

@snapwich whoa, good find.
Maybe call it utils.cloneByJsonParse or something, just to discourage use. cloneDeep sounds like it should copy everything, but parse drops functions.

@snapwich
Copy link
Collaborator Author

That's a good call. I've updated it to be called cloneJson

@mkendall07 mkendall07 requested a review from matthewlane March 21, 2017 19:46
Copy link
Collaborator

@matthewlane matthewlane left a comment

Choose a reason for hiding this comment

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

👍

@matthewlane matthewlane merged commit 4c9d6f6 into prebid:master Mar 21, 2017
@snapwich snapwich mentioned this pull request May 3, 2017
1 task
dluxemburg pushed a commit to Genius/Prebid.js that referenced this pull request Jul 17, 2018
…1055)

* replace broken utils.extend functionality with object.assign

* moved copy function to utils as cloneJson
@robertrmartinez robertrmartinez deleted the object-assign branch July 5, 2023 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants