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

Universal ID Module - Dev Review PR only, do not merge!!! #31

Closed
wants to merge 94 commits into from

Conversation

idettman
Copy link

@idettman idettman commented Dec 12, 2018

Universal ID Module

Dev Review PR only, do not merge!!!

Related To

Requirements/Spec
HB-3495

            pbjs.setConfig({
                debug: true,
                usersync: {
                    universalIds: [{
                        name: "unifiedId",
                        params: {
                            partner: "prebid",
                            url: "http://match.adsrvr.org/track/rid?ttd_pid=prebid&fmt=json"
                        },
                        storage: {
                            type: "cookie",
                            name: "unifiedid",
                            expires: 60
                        }
                    }, {
                        name: "pubCommonId",
                        storage: {
                            type: "cookie",
                            name: "pubcid",
                            expires: 60
                        }
                    }],
                    syncDelay: 5000
                }
            });

Isaac Dettman and others added 20 commits December 10, 2018 11:52
…ow to handle both 'value' and 'storage' existing in config
@idettman idettman self-assigned this Dec 13, 2018
@idettman idettman assigned idettman and unassigned idettman Jan 4, 2019
@goosemanjack
Copy link

I've been playing with integrating DigiTrust into this design. From what I can tell I'd have to add DigiTrust linkage as an embedded Submodule in the universalId.js file and update associated unit tests. Additionally the main DigiTrust library would need to be included or concatenated onto the prebid.js source.

And just to verify my understanding, all IDs and ID providers are sub-objects under the Universal Id (unified ID?). I also see a dependency injection branch. I haven't reviewed the code, but if I could keep my submodule injection in a separate source file that would be preferable.

Thanks for any guidance.

@idettman
Copy link
Author

idettman commented Jan 5, 2019

@goosemanjack Yes, to add support for a DigiTrust ID, a submodule (implementing a configKey property and 'decode' and 'getId' functions) would be added to the submodules array in this current form. You could possibly create the submodule in a separate file that exports the submodule object, and we can import/require, then add it in the universalId.js submodules array. Something like:

import digiTrustSubmodule from './digitrust.js';

const pubCommonIdSubmodule = {...}
const unifiedIdSubmodule = {...}

const submodules = [
  pubCommonIdSubmodule,
  unifiedIdSubmodule,
  digiTrustSubmodule
]

Note: if we decide to have submodules in separate files, we would create a folder in modules named universalId which would contain an index.js and any submodule js files (in your case something like digitrust.js).

I'll check with my team and reply back asap (prob Monday).

@bretg
Copy link

bretg commented Jan 7, 2019

@goosemanjack - how much code are we talking about? Every system included in the universal ID module must be small -- a couple hundred bytes. This is because everyone who opts in to the module will get the code for all ID systems regardless of which ones they actually use.

If DigiTrust support won't fit in a small footprint, it will need to be a separate module. This is fine, and would still get you PBJS support. We may be able to come up with a name for this particular module to back off the "Universal" claim. e.g. "Combined ID Module".

@goosemanjack
Copy link

I have a first rev of the DigiTrust integration for Universal Id. It's just under 3k before minification, and most of that code is templated off the syncDelay bits on Unified Id submodule, so it could get tighter. The module returns an empty if the broader DigiTrust framework isn't present, so it shouldn't add much to non-DigiTrust members.

How would you like the submodule and changes delivered for review? I could probably put together a patch, but it is (obviously) dependent on this patch.

Thanks.

@idettman
Copy link
Author

You can send the .js files, it doesn't need to be a patch.

@goosemanjack
Copy link

I'm just waiting for a DigiTrust release to go live that includes a bug fix we need, then I'll send in the JS files and a readme after testing again.

@bretg
Copy link

bretg commented Jan 23, 2019

@goosemanjack - please describe the usage of this proposed additional to the module. Sounds like a publisher would need to include another piece of digitrust code in their pages on top of Prebid.js and this module?

If that's the case, I propose Digitrust should be its own separate module. We've been talking about this for a long time -- if the Digitrust client library were available as an NPM, it would be easier to provide a way for publishers to pull the DT code and any necessary glue into their Prebid.js package.

If necessary, we can change the name from "Universal ID" module to something else. e.g. "Combined User ID" module. Not as sexy, but if DT isn't going to fit in it, more accurate.

@goosemanjack
Copy link

We have DigiTrust published to the npm repository as "digitrust". Code and examples in the below zip are based upon the same in this PR. There is a readme that goes over all details.

This integrates the DigiTrust Id into the Prebid pipeline in a way the hopefully conforms with your vision. The one thing I am considering is building support for the full DigiTrust.initialize into the UniversalId integration. I feel the main DigiTrust package should remain an independent project since it serves a different use case than Prebid.

Download the zip: release_digi.v0_5.zip
from here or github
release_digi.v0_5.zip

https://github.com/goosemanjack/Prebid.js/releases/tag/v1.4.0.1

@bretg
Copy link

bretg commented Jan 23, 2019

@goosemanjack - spoke with Matt Kendall. We agreed that if you guys are going to require outside code anyhow, you should be able to get your in-module code down to something very small - a couple hundred bytes before minification. Can you move out everything but the check for the existence of the required code?

You might want to take a look at the documentation we've drafted and start thinking about how DigiTrust will fit into it.

https://staging.prebid.org/dev-docs/modules/universalid.html

@goosemanjack
Copy link

I've looked over the integration code one more time. If I embed directly in the universalId.js module we can trim 12 lines since the call to hasGDPRConsent will be accessibly within the closure. I could also trim another 14 lines if we ignore the syncDelay passed in to getId like the pubCommonId code does. As I don't have enough background on using Prebid to know all the ins and outs of ignoring this parameter I'm a little hesitant. We do cross-domain postMessage and in some cases an XHR call to sync the ID, so the delay may be required if it isn't just a testing parameter.

I'm loath to embed this code within the universalId.js module just for maintenance reasons. If it's just a matter of the hasGDPRConsent could we get that function moved to the gdprDataHandler object? Another option would be to abstract out and centralize both syncDelay handling and hasGDPRConsent checks into the core universalId module prior to invoking getId such that implementations don't have to duplicate this logic. Then I could trim down the code to only handling init checking and getId passthrough.

@goosemanjack
Copy link

We have an updated package for the DigiTrust integration that can be used with or without the main DigiTrust library present. It is under review internally. You can also download the zip package here:
https://github.com/goosemanjack/Prebid.js/releases/download/digi_v0_6/release_digi.v0_6.zip

@idettman idettman closed this Feb 12, 2019
@idettman idettman deleted the universal-id-module branch June 5, 2019 23:41
robertrmartinez pushed a commit that referenced this pull request Sep 16, 2021
…ata (prebid#7167)

* DL-1746: Remove trId and use notid instead (#31)

* Remove trId and use notid instead
* Update version to 0.7.3
* Set default value to empty string

Co-authored-by: SublimeJeremy <jeremy@sublimeskinz.com>

* DL-1780: Fix pubtimeout (#32)

* Update timeout value with onTimeout data
* Add test for onTimeout
* Update state with timeout asap

Co-authored-by: SublimeJeremy <jeremy@sublimeskinz.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants