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

LiveIntent User ID Module: Eliminating live-connect NPM Dependency #12167

Merged
merged 17 commits into from
Sep 30, 2024
Merged

LiveIntent User ID Module: Eliminating live-connect NPM Dependency #12167

merged 17 commits into from
Sep 30, 2024
+1,034 −416

Conversation

3link
Copy link
Contributor

@3link 3link commented Aug 23, 2024 •
edited
Loading

Type of change

  • Feature

Description of change

Allows to use the LiveIntent user ID module with an instance of LiveConnect v3 present on the page instead of inlining the LiveConnect NPM module into the user ID module as described here: #11541.

The goal is to support both standard/minimal mode as well as the external mode during the migration phase. Once all our customers have migrated away from using standard/minimal mode, we will deprecate this mode and delete the LiveConnect NPM module dependency from Prebid together with all the code that depends on it. That would include moving the code from libraries/liveIntentId back into modules/liveintentIdSystem as we would not need to differentiate between two module implementations.

Other information

@github-actions GitHub Actions
Copy link

Tread carefully! This PR adds 8 linter errors (possibly disabled through directives):

  • libraries/liveIntentId/externalIdSystem.js (+1 error)
  • libraries/liveIntentId/idSystem.js (+3 errors)
  • modules/liveIntentIdSystem.js (+4 errors)

@github-actions GitHub Actions
Copy link

Tread carefully! This PR adds 6 linter errors (possibly disabled through directives):

  • libraries/liveIntentId/externalIdSystem.js (+1 error)
  • libraries/liveIntentId/idSystem.js (+1 error)
  • modules/liveIntentIdSystem.js (+4 errors)

@github-actions GitHub Actions
Copy link

Tread carefully! This PR adds 5 linter errors (possibly disabled through directives):

  • libraries/liveIntentId/idSystem.js (+1 error)
  • modules/liveIntentIdSystem.js (+4 errors)

@github-actions GitHub Actions
Copy link

Tread carefully! This PR adds 4 linter errors (possibly disabled through directives):

  • libraries/liveIntentId/idSystem.js (+1 error)
  • modules/liveIntentIdSystem.js (+3 errors)

@github-actions GitHub Actions
Copy link

Tread carefully! This PR adds 4 linter errors (possibly disabled through directives):

  • libraries/liveIntentId/idSystem.js (+1 error)
  • modules/liveIntentIdSystem.js (+3 errors)

@3link 3link marked this pull request as ready for review August 26, 2024 09:14
@3link
Copy link
Contributor Author

3link commented Sep 4, 2024

@jlquaccia I have a question about possible ways to extend our user id module that depends on the external LiveConnect. Would it be possible to extend the module configuration such that a URL can be provided from which to download the LiveConnect script and the user id module would then embed this script on the page? If no such URL is provided in the configuration, nothing will be embedded.

That would make the switch for our customers smoother as they will just need to add an additional config to our user id module and it will automatically deploy the script.

@jlquaccia
Copy link
Collaborator

@jlquaccia I have a question about possible ways to extend our user id module that depends on the external LiveConnect. Would it be possible to extend the module configuration such that a URL can be provided from which to download the LiveConnect script and the user id module would then embed this script on the page? If no such URL is provided in the configuration, nothing will be embedded.

That would make the switch for our customers smoother as they will just need to add an additional config to our user id module and it will automatically deploy the script.

Overall, I think the changes on this PR seem fine to me. In terms of allowing a URL to be provided via module configuration, I'm not sure if that is a direction that Prebid would like to go in since it seems it could open some security related scenarios. @ncolletti @patmmccann what are your thoughts

@3link
Copy link
Contributor Author

3link commented Sep 26, 2024

@jlquaccia I have a question about possible ways to extend our user id module that depends on the external LiveConnect. Would it be possible to extend the module configuration such that a URL can be provided from which to download the LiveConnect script and the user id module would then embed this script on the page? If no such URL is provided in the configuration, nothing will be embedded.
That would make the switch for our customers smoother as they will just need to add an additional config to our user id module and it will automatically deploy the script.

Overall, I think the changes on this PR seem fine to me. In terms of allowing a URL to be provided via module configuration, I'm not sure if that is a direction that Prebid would like to go in since it seems it could open some security related scenarios. @ncolletti @patmmccann what are your thoughts

Just to provide more context here: our customers would now be required to place our LiveConnect script on their domains in order for our user ID sub-module to work. We have customers with thousands of domains and it would make upgrading to the newer Prebid/LiveIntent sub-module version much easier for them if instead of setting up LiveConnect on each of their pages manually, they could configure their URL for the LiveConnect script directly in our user ID module.

@ncolletti
Copy link
Contributor

@3link As far as I'm aware Prebid has been upfront about not allowing external code to be loaded other than from RTD Modules where a contract/registration to acknowledge the contents of the JS by the publisher is signed prior. While cumbersome, I think this configuration is preferred by Prebid.

Somewhat recent guidance on this topic: #9570

Any case, I don't know if a change should happen in this iteration of your adapter. It's best to bring this up to the Identity PMC group for further discussion

@3link
Copy link
Contributor Author

3link commented Sep 27, 2024

As far as I'm aware Prebid has been upfront about not allowing external code to be loaded other than from RTD Modules where a contract/registration to acknowledge the contents of the JS by the publisher is signed prior. While cumbersome, I think this configuration is preferred by Prebid.

Somewhat recent guidance on this topic: #9570

Any case, I don't know if a change should happen in this iteration of your adapter. It's best to bring this up to the Identity PMC group for further discussion

@ncolletti That makes sense. Thank you for the link!

I think it makes sense to keep these two topics separate and merge this version of the module as is for now.

@ncolletti
Copy link
Contributor

@jlquaccia looks good to me, will let you give a final sign off and merge

@jlquaccia
Copy link
Collaborator

@ncolletti yep, looks good to me too. Merging now..

@jlquaccia jlquaccia merged commit bdab819 into prebid:master Sep 30, 2024
6 checks passed
s-solodovnikov
s-solodovnikov reviewed Oct 16, 2024
View reviewed changes
libraries/liveIntentId/idSystem.js
import { LiveConnect } from 'live-connect-js'; // eslint-disable-line prebid/validate-imports
import { getStorageManager } from '../../src/storageManager.js';
import { MODULE_TYPE_UID } from '../../src/activities/modules.js';
import { DEFAULT_AJAX_TIMEOUT, MODULE_NAME, composeIdObject, eids, DEFAULT_DELAY, GVLID, PRIMARY_IDS, parseRequestedAttributes } from './shared.js'
Copy link

Choose a reason for hiding this comment

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

Hi. You are importing the DEFAULT_DELAY const but it does not exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@s-solodovnikov Good catch! Thank you notifying me. Will fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#12334

@3link 3link mentioned this pull request Oct 16, 2024
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@s-solodovnikov s-solodovnikov s-solodovnikov left review comments

@jlquaccia jlquaccia jlquaccia approved these changes

@ncolletti ncolletti ncolletti approved these changes

Assignees

@jlquaccia jlquaccia

@ncolletti ncolletti

Labels
maintenance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

LiveIntent User ID Module: Eliminating live-connect NPM Dependency.
6 participants
@3link @jlquaccia @ncolletti @s-solodovnikov @jsnellbaker @peixunzhang