-
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
Prebid Core: add documentResolver callback and allow the user to supply a different document object to render #8262
Changes from all commits
4f6d88b
bbc95b8
0108444
0a7d686
a5dd667
2d17a66
a3aa31b
91bef73
1b10e89
58b7af1
6b14f5e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
import { expect } from 'chai'; | ||
import { Renderer } from 'src/Renderer.js'; | ||
import { Renderer, executeRenderer } from 'src/Renderer.js'; | ||
import * as utils from 'src/utils.js'; | ||
import { loadExternalScript } from 'src/adloader.js'; | ||
require('test/mocks/adloaderStub.js'); | ||
|
@@ -212,5 +212,20 @@ describe('Renderer', function () { | |
testRenderer.render() | ||
expect(loadExternalScript.called).to.be.true; | ||
}); | ||
|
||
it('call\'s documentResolver when configured', function () { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a case to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
const documentResolver = sinon.spy(function(bid, sDoc, tDoc) { | ||
return document; | ||
}); | ||
|
||
let testRenderer = Renderer.install({ | ||
url: 'https://httpbin.org/post', | ||
config: { documentResolver: documentResolver } | ||
}); | ||
|
||
executeRenderer(testRenderer, {}, {}); | ||
|
||
expect(documentResolver.called).to.be.true; | ||
}); | ||
}); | ||
}); |
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.
here you check that
doc
andcacheObject
are defined before using them. It doesn't hurt, except maybe for adding a couple bytes, but they should always be defined here - conceptually, if you're going to check for that, it would make more sense to throw an error when they are undefined (but I wouldn't do that either).It's not that consequential so I don't mind if it stays this way, just noting it as an odd choice IMO.
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.
It depends,
doc
being not set defaults to the loadeddocument
, which in theory does also happen down the line ininsertElement
and some other methods. But now it's specific from here on. But i do agree it could also be an Argument Exception if there is a need to be specific about this.The
cacheObject
could theoeratically be gone for what ever reason it's resolved there. This could actually be an error, since thetag
property won't be set, but the callback would flag the object as loaded, which is sort of inconsistent. But not failing there and just loading the script, without failing there is i think not that troublesome.