Skip to content

Submodule system using hooks#3924

Merged
jaiminpanchal27 merged 21 commits intomasterfrom
prebid-core-bundle-fix
Jul 2, 2019
Merged

Submodule system using hooks#3924
jaiminpanchal27 merged 21 commits intomasterfrom
prebid-core-bundle-fix

Conversation

@snapwich
Copy link
Collaborator

@snapwich snapwich commented Jun 18, 2019

Type of change

  • Bugfix

Description of change

Only include src dir and few node_modules inside of prebid-core bundle.

Other information

related to #3908

@snapwich

This comment has been minimized.

Sign in to view
@snapwich
Copy link
Collaborator Author

snapwich commented Jun 19, 2019

Update the prebid-core hooks module with a module and submodule helper method which permits an install function for the parent module and the ability to pass data to the installer from the child module. This should allow for more sharing of functionality between child and parent modules without requiring either as static imports.

Also added the ./modules/.submodules.json to make the gulp bundler aware of these relationships. When using gulp build --modules=digiTrustIdSystem for example it will check the .submodules.json and see that digiTrust is a child of the userId module, so it will automatically include the parent module as well. Since they're using async hooks that are queued behind the scenes, load order of the modules doesn't matter.

@snapwich snapwich changed the title prebid-core only contains src and a few node_modules Submodule system using hooks Jun 19, 2019
@snapwich snapwich mentioned this pull request Jun 19, 2019
Closed
1 task
idettman
idettman approved these changes Jun 20, 2019
View reviewed changes
Copy link
Contributor

@idettman idettman left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Updated with userId submodule changes

@idettman idettman added needs 2nd review Core module updates require two approvals from the core team and removed needs review labels Jun 20, 2019
jsnellbaker
jsnellbaker approved these changes Jun 24, 2019
View reviewed changes
Copy link
Collaborator

@jsnellbaker jsnellbaker left a comment

Choose a reason for hiding this comment

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

LGTM

@idettman idettman added on hold and removed needs 2nd review Core module updates require two approvals from the core team labels Jun 24, 2019
@idettman

This comment has been minimized.

Sign in to view
@snapwich
Copy link
Collaborator Author

snapwich commented Jun 24, 2019

it might be worthwhile including the required changes to freewheel and adpod in this pull-request as well so we aren't left with a partial fix in master for module issues: @jsnellbaker @jaiminpanchal27

@jaiminpanchal27
Copy link
Collaborator

jaiminpanchal27 commented Jun 24, 2019

@snapwich I am starting that work today. I will update you as soon as it is done. Can I push to this branch or we create one new branch and create our PR's against that branch ?

@snapwich
Copy link
Collaborator Author

snapwich commented Jun 24, 2019

@jaiminpanchal27 you should be able to push to this branch, but a PR against this branch works as well. Whatever is easier for you guys!

@idettman idettman added the needs 2nd review Core module updates require two approvals from the core team label Jun 27, 2019
@idettman idettman removed their request for review June 27, 2019 17:59
@idettman

This comment has been minimized.

Sign in to view
Jaimin Panchal added 2 commits June 27, 2019 14:47
@jaiminpanchal27
Copy link
Collaborator

jaiminpanchal27 commented Jun 27, 2019

I have pushed adpod related changes. @jsnellbaker Please have a look and test
Added on hold label so we do not merge this without testing all prebid submodules.

snapwich
snapwich commented Jun 28, 2019
View reviewed changes
modules/.submodules.json Outdated
"pubCommonIdSystem",
"unifiedIdSystem",
"id5IdSystem",
"unifiedIdSystem"
Copy link
Collaborator Author

@snapwich snapwich Jun 28, 2019

Choose a reason for hiding this comment

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

I think someone handled a merge improperly here. Duplicate and removed modules are showing up now...

Might want to check to make sure no other changes were accidentally reverted as well.

Copy link
Contributor

@idettman idettman Jun 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah this is supposed to be:

{
  "userId": [
    "digiTrustIdSystem",
    "id5IdSystem"
  ],
  "adpod": [
    "freeWheelAdserverVideo"
  ]
}

I checked the rest of the userId and it's releated files, and all look good. I'll commit a fix for the .submodules.json

@jsnellbaker jsnellbaker self-requested a review July 1, 2019 18:18
@jaiminpanchal27 jaiminpanchal27 merged commit 113cfe9 into master Jul 2, 2019
@idettman idettman added LGTM and removed needs 2nd review Core module updates require two approvals from the core team on hold labels Jul 15, 2019
@idettman idettman removed the request for review from robertrmartinez July 19, 2019 18:22
@pycnvr pycnvr mentioned this pull request Jul 24, 2019
Merged
10 tasks
VideoReach pushed a commit to VideoReach/Prebid.js that referenced this pull request Aug 1, 2019
@snapwich
Submodule system using hooks (prebid#3924)
40676f4 
* prebid-core only contains src and a few node_modules

* add back removed neverBundle to webpack build

* add module and submodule hooks

* allow vargs for submodules for flexibility

* fix jsdoc type syntax

* updated id5 userid submodule for submodule bundle size duplication fix

* add id5 userid submodule to .submodules

* fix opt out logic

* spelling fix to comment

* update to automatically include 'pubcommon' and 'unifiedid' (uncomment to optional submodule for prebid3.0)

* additional update to automatically include 'pubcommon' and 'unifiedid'

* additional update to automatically include 'pubcommon' and 'unifiedid'

* merged differences from master

* adpod changes to support submodules

* fix --modules argument with .json to work correctly with submodules

* fix to remove included and duplicated submodules
sa1omon pushed a commit to gamoshi/Prebid.js that referenced this pull request Nov 28, 2019
@snapwich
Submodule system using hooks (prebid#3924)
ecc8b2a 
* prebid-core only contains src and a few node_modules

* add back removed neverBundle to webpack build

* add module and submodule hooks

* allow vargs for submodules for flexibility

* fix jsdoc type syntax

* updated id5 userid submodule for submodule bundle size duplication fix

* add id5 userid submodule to .submodules

* fix opt out logic

* spelling fix to comment

* update to automatically include 'pubcommon' and 'unifiedid' (uncomment to optional submodule for prebid3.0)

* additional update to automatically include 'pubcommon' and 'unifiedid'

* additional update to automatically include 'pubcommon' and 'unifiedid'

* merged differences from master

* adpod changes to support submodules

* fix --modules argument with .json to work correctly with submodules

* fix to remove included and duplicated submodules
@idettman idettman deleted the prebid-core-bundle-fix branch March 8, 2021 16:47
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

@jsnellbaker jsnellbaker jsnellbaker approved these changes

+1 more reviewer

@idettman idettman idettman approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@idettman idettman

@jaiminpanchal27 jaiminpanchal27

@robertrmartinez robertrmartinez

@jsnellbaker jsnellbaker

Labels

LGTM

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@snapwich @idettman @jaiminpanchal27 @jsnellbaker @robertrmartinez

Comments