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

Refactor UserId Module with external submodules #3831

Merged
merged 1 commit into from
May 21, 2019
Merged

Refactor UserId Module with external submodules #3831

merged 1 commit into from
May 21, 2019

Conversation

idettman
Copy link
Contributor

@idettman idettman commented May 15, 2019 •
edited
Loading

Type of change

  • Other

Description of change

Support external userId sub-modules

  • PBJS userId sub-modules are refactored as separate files.

Other updates

  • Fix unifiedId HTTPS support.
    issue 3796
  • Fix allow camelcase userSync and lowercase usersync,
    The plan is to support both usersync and userSync until Prebid.js 3.0, and then usersync will be deprecated.
    issue 3818

Other information

issue 3818
issue 3796

@idettman idettman self-assigned this May 15, 2019
@idettman idettman added the needs 2nd review Core module updates require two approvals from the core team label May 15, 2019
@idettman idettman force-pushed the userid-external-submodules branch 3 times, most recently from 8f68e2f to 5ab0c03 Compare May 16, 2019 15:45
jaiminpanchal27
jaiminpanchal27 requested changes May 16, 2019
View reviewed changes
Copy link
Collaborator

@jaiminpanchal27 jaiminpanchal27 left a comment

Choose a reason for hiding this comment

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

Overall looks good. Please see comments

modules/idSystemUnifiedId.js Outdated

import * as utils from '../src/utils.js'
import {ajax} from '../src/ajax.js';
import {MODULE_NAME} from './userId.js'
Copy link
Collaborator

Choose a reason for hiding this comment

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

On running gulp serve, hello_world page throws javascript error. It is something because of this circular dependency.

modules/idSystemPubCommonId.js Outdated
/**
* This module adds PubCommonId to the User ID module
* The {@link module:userId} module is required
* @module idSystemPubCommonId
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry for the confusion here. Shouldn't the name of module be like <name>IdSystem.js to be consistent with our naming scheme.

modules/userId.js Outdated
}

/**
* @param {PrebidConfig} config
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typedef missing for PrebidConfig. Also can you add some description to this function

test/spec/modules/userId_spec.js
utils.setCookie('unifiedid', '', EXPIRED_COOKIE_DATE);
done();
}, {adUnits});
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you also one more test case should add new id system to make sure new id systems will be added correctly. e.g: digitrust is going to be next one

@idettman idettman force-pushed the userid-external-submodules branch 2 times, most recently from 70e00af to fcfa6b5 Compare May 16, 2019 20:49
@idettman idettman closed this May 16, 2019
@idettman idettman force-pushed the userid-external-submodules branch from fcfa6b5 to ad7b59d Compare May 16, 2019 20:55
@idettman idettman reopened this May 16, 2019
@idettman idettman force-pushed the userid-external-submodules branch 2 times, most recently from d925747 to 409066e Compare May 17, 2019 00:27
@idettman
Copy link
Contributor Author

requested changes are ready for review

@idettman idettman force-pushed the userid-external-submodules branch from 409066e to 642abb2 Compare May 17, 2019 05:18
@bretg bretg mentioned this pull request May 20, 2019
Closed
bretg
bretg approved these changes May 20, 2019
View reviewed changes
Copy link
Collaborator

@bretg bretg left a comment

Choose a reason for hiding this comment

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

Confirmed the usersync case sensitivity and adsvr.org https issues.

@bretg
Copy link
Collaborator

bretg commented May 20, 2019

@idettman or @jaiminpanchal27 - how would people include a given module into this new approach? I'd like to have a consistent naming structure like:

gulp build --modules=userId,userId-digitrust,userId-id5

i.e. can we enforce that all sub-modules start with "userId-" ?

harpere
harpere approved these changes May 21, 2019
View reviewed changes
Copy link
Collaborator

@harpere harpere left a comment

Choose a reason for hiding this comment

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

LGTM

@bretg bretg removed the request for review from robertrmartinez May 21, 2019 19:00
@bretg
Copy link
Collaborator

bretg commented May 21, 2019

Spoke with the team. The convention will be that all of these modules will be suffixed with *IdSystem. So eg.

gulp build --modules=pubcommonIdSystem,unifiedIdSystem

For 2.x, these two modules are grandfathered and automatically included. That should change in 3.0

@bretg bretg merged commit d55b253 into prebid:master May 21, 2019
@idettman
Copy link
Contributor Author

@bretg Here's an example showing the unifiedId submodule converted as an optional submodule:

import {attachIdSystem} from 'userId.js'

export const unifiedIdSubmodule = {
  name: 'unifiedId',
  decode(value) {  ... },
  getId(configParams) { ... }
};

// add this submodule to userId module
attachIdSystem(unifiedIdSubmodule);

@idettman idettman deleted the userid-external-submodules branch June 5, 2019 23:40
@idettman idettman removed the needs 2nd review Core module updates require two approvals from the core team label Jun 6, 2019
VideoReach pushed a commit to VideoReach/Prebid.js that referenced this pull request Aug 1, 2019
support external userId sub-modules (prebid#3831)
bb55a65
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaiminpanchal27 jaiminpanchal27 jaiminpanchal27 approved these changes

@harpere harpere harpere approved these changes

@bretg bretg bretg approved these changes

Assignees

@idettman idettman

@jaiminpanchal27 jaiminpanchal27

Labels
LGTM
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@idettman @bretg @harpere @jaiminpanchal27