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

sharethrough: wait for document.body to exist #1026

Closed
wants to merge 1 commit into from

Conversation

kitwestneat
Copy link
Contributor

Type of change

  • [x ] Bugfix

Description of change

If document.body doesn't exist yet, wait for content loaded event
from document before adding iframe.

Other information

Fixes issue #1025

If document.body doesn't exist yet, wait for content loaded event
from document before adding iframe.
@snapwich
Copy link
Collaborator

snapwich commented Mar 6, 2017

Is there a reason you use an iFrame to load JSONP content rather than a script tag? Also, Prebid.js provides adloader#loadScript to load JSONP data.

I'm pretty sure an iFrame is going to both slow down the auction as well as put you guys at a disadvantage as far as response time.

@kitwestneat
Copy link
Contributor Author

kitwestneat commented Mar 6, 2017

@snapwich I'm not a sharethrough developer, so I can't comment specifically on their loading procedure. Using JSONP directly instead of their iframe solution seems a lot simpler to me as well. With the current parameters, their bid server returns HTML containing a script tag with the callback, so not JSONP per se (they use the term ijson).

It looks like @rizhang made the initial commit, so perhaps they can comment on it?

@rizhang
Copy link
Contributor

rizhang commented Mar 9, 2017

We used an iframe because that is how are endpoint is meant to be consumed and we didn't want to do any backend work to support jsonp.

I made a PR to use ajax instead of an iframe hoping that it will speed up our response times. #1042

@kitwestneat
Copy link
Contributor Author

@rizhang thanks for your PR and backend change, it seems to work so I'll close this

@kitwestneat kitwestneat closed this Mar 9, 2017
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