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

LiveIntent Id Submodule: Update live-connect build dependency to 2.3.1 #8198

Merged
merged 6 commits into from
Apr 2, 2022

Conversation

3link
Copy link
Contributor

@3link 3link commented Mar 18, 2022

Type of change

  • Bugfix
  • Feature
  • New bidder adapter
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Does this change affect user-facing APIs or examples documented on http://prebid.org?
  • Other

Description of change

Live Connect library update. Changes in library relevant for LiveIntent's userId module:

  • remove usage (get/set) of legacy _litra_id.X cookies

@patmmccann
Copy link
Collaborator

What does the new library do?

@3link
Copy link
Contributor Author

3link commented Mar 22, 2022

@patmmccann primarily, the new library version removes the usage of a legacy cookie. The feature set used for our Prebid module remains unchanged.

@3link 3link marked this pull request as ready for review March 22, 2022 12:11
@ChrisHuie ChrisHuie self-requested a review March 22, 2022 19:28
@ChrisHuie ChrisHuie self-assigned this Mar 22, 2022
@ChrisHuie
Copy link
Collaborator

@3link can you please remove your package.json file from this pr 🙏

@3link
Copy link
Contributor Author

3link commented Mar 23, 2022

@ChrisHuie the package.json change contains the update of our LiveConnect library that is used by our user id module. This change is the reason for this PR. Or did you rather mean the changes in package-lock.json?

@patmmccann patmmccann changed the title Update live-connect to 2.3.1 LiveIntent Id Submodule: Update live-connect build dependency to 2.3.1 Mar 23, 2022
@ChrisHuie
Copy link
Collaborator

@ChrisHuie the package.json change contains the update of our LiveConnect library that is used by our user id module. This change is the reason for this PR. Or did you rather mean the changes in package-lock.json?

Can you please pull in most recent merges then on the main branch and I can merge :) 🙏

@3link
Copy link
Contributor Author

3link commented Mar 29, 2022

Can you please pull in most recent merges then on the main branch and I can merge :) 🙏

Sure! Done.

@patmmccann
Copy link
Collaborator

@3link @ChrisHuie this still has changes on package lock, not ready for merge

@3link
Copy link
Contributor Author

3link commented Mar 29, 2022

this still has changes on package lock, not ready for merge
@patmmccann @ChrisHuie Which of the changes shall be removed?

@ChrisHuie
Copy link
Collaborator

@3link since you are updating the dependencies it looks fine to me other than the release number probably needing to be updated again. @patmmccann is there something else in the package-lock you are referencing?

@ChrisHuie ChrisHuie removed the request for review from dgirardi March 30, 2022 19:31
Copy link
Collaborator

@dgirardi dgirardi left a comment

Choose a reason for hiding this comment

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

The changes in here LGTM, but since the liveIntent module effectively pulls in an external dependency into Prebid, does that mean we should be reviewing all updates that went into live-connect-js from 2.0.0 to 2.3.1? (This is a probably a policy question for @patmmccann - was the live-connect-js codebase reviewed originally?)

@@ -211,7 +211,7 @@ describe('LiveIntentId', function() {

it('should include the LiveConnect identifier and additional Identifiers to resolve', function() {
const oldCookie = 'a-xxxx--123e4567-e89b-12d3-a456-426655440000'
getDataFromLocalStorageStub.withArgs('_li_duid').returns(oldCookie);
getCookieStub.withArgs('_lc2_fpi').returns(oldCookie);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given the PR description I find it surprising that you'd change these from local storage to cookies - but if it makes sense to you (& the rest of the maintainers) I have no issue with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dgirardi The resulting behaviour did not change. A step for building the request was refactored and the dependency on the _lc2_fpi cookie being present became more obvious in the test. It was there before too, but it was enough to stub getDataFromLocalStorage.

@3link
Copy link
Contributor Author

3link commented Mar 31, 2022

since you are updating the dependencies it looks fine to me other than the release number probably needing to be updated again

Done.

@patmmccann
Copy link
Collaborator

@3link could you link to change log between 2.0.0 and 2.3.1 for reviewers' convenience?

Copy link
Collaborator

@patmmccann patmmccann left a comment

Choose a reason for hiding this comment

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

@patmmccann patmmccann merged commit 4a83fff into prebid:master Apr 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants