-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Debug update and remove setConfig hook #4007
Conversation
Please hold on this PR - there is an issue that needs to be addressed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a fix. The issue is caused by us having a circular dependency between the utils module and the config module. Circular dependencies are actually allowed but if you import a reference and immediately execute it, it can cause issues with the binding's value being undefined depending on the load order (hence why all the utils.logMessage
and such in config are no problem since they're not immediately called but utils.getParameterByName
was an issue since it was immediately invoked).
I attempted breaking out some of the query string utilities into their own module and then just re-exporting them in the utils module (to prevent a huge refactor) however sinon apparently has issues stubbing re-exported bindings compared with regular es6 exports... Really weird.
I'm not a huge fan of this solution because I just changed config to use the url.js#parseQS
function I found already in our code but the circular dependency still exists. However, since parseQS
is not part of the circular dependency the issue doesn't present itself. I'm also not a fan because url.js#parseQS
and utils.js#getParameterByName
basically do the same thing. But I guess this is good enough for now.
* Debug update and remove setConfig hook * fix config failing on circular dependency with utils
* Debug update and remove setConfig hook * fix config failing on circular dependency with utils
* Debug update and remove setConfig hook * fix config failing on circular dependency with utils
Type of change
Description of change
A re-submission of an older PR #3714 that was reverted from
master
via #3736.Re-copying the description of the above ticket here for reference:
With the implementation of the new hooks library we added a
ready
functionality that would allowasync
hooks to be queued until the hooks were declaredready
inpbjs.processQueue
. This causedconfig.setConfig
calls to be queued andconfig.getConfigs
being called beforepbjs.processQueue
were not getting data (such as the call for printing prebid info in #3700). I updated the DEFAULT_DEBUG to be set immediately rather than relying onconfig.setConfig
in theutils
library, which I think makes a little more sense.Also, the
pbjs.setConfig
hook was only being used for thepre1api
module, which was removed in 2.0 so I think it's best we just remove the hook, which I've also done.Other information
Fixes #3700 and #3997