Core & multiple modules: refactor usage of Promise to avoid deferrals when possible

[dgirardi]

Type of change

• Refactoring (no functional changes, no api changes)

Description of change

This refacts all usage of Promise in the "critical" path (from requestBids to renderAd) to run synchronously if possible, in light of this discussion.

The approach is swapping Promise with a custom GreedyPromise that will avoid giving up control if it can. Apart for the difference in callback execution timing, the two should be interchangeable.

[patmmccann]

@dgirardi is it simple to prepare a backport of this to legacy?

[dgirardi]

@patmmccann it shouldn't be too hard, but I'll wait for a review on this before attempting it.

[jsnellbaker]

From what I can tell/understand, I think this all looks okay.

As a possible thought, given the shift this PR is introducing to prebid.js' internal Promise model to force a synchronous execution - is there any value to setting up a separate standard interface/model that uses the normal Promise behavior?

I saw with some of files that the functions use a base Promise to help with the their execution (or debugging). Should those base Promise use-cases be something that we want to build a control mechanism within Prebid.js - to future-proof any potential future changes (like this)? Or is that not really needed/not a big deal if it were to happen.

[dgirardi]

@jsnellbaker I'm not sure I understand your question. Do you mean something like marking "normal" promises as a lint error? I'm not against it, but also not convinced it's going to be that useful (because in some situations normal promises are the correct choice, so ultimately the contributor needs to know the difference).

[snapwich]

this looks fine to me and yeah, as you say, regular Promise usage should be the standard unless there's a good reason to use the GreedyPromise, which i think is mostly reserved for prebid-core usage (or hooks that interact with prebid-core).

good work on the Promise test suite, really gives me confidence that the replacement functions as expected!

[jsnellbaker]

I was thinking of how we have an internal interface for ajax calls that we ask everyone to use (to control how the calls are executed/maintained/etc) instead of everyone creating their own references; so my previous thought would be mimicking that setup/usage behavior in Prebid.js to setup something for 'normal' Promises, for everyone to use in a standardized way.

But as I said, it was just a potential thought.

[dgirardi]

@jsnellbaker a wrapper around Promise would have no substance, I think, so at the moment I don't see the utility. (ajax would historically provide an interface for xmlhttprequest, and has some prebid-specific logic for timeouts and so on, Promise would just be Promise, renamed).

[bjorn-lw]

It seems as if this change is incompatible with bluebirdjs. When using Prebid.js 7.7.x+ we get the following error on one our publisher's sites:

Uncaught TypeError: the promise constructor cannot be invoked directly

and the auction doesn't proceed.

We believe it's due to GreedyPromise extending Promise:

http://bluebirdjs.com/docs/error-explanations.html#2.-you-are-trying-to-subclass-promise

Unfortunately we can't simply ask the publisher to remove bluebirdjs so it would be awesome if it was possible to find a way around this.

It doesn't seem like something easy to do, but the publisher is a major one and I would assume this could happen elsewhere as well.

[patmmccann]

Bluebird is not recommended on older browsers. Your publisher should not be loading modern prebid and bluebird together, they should load old prebid and bluebird for old browsers, no bluebird and modern prebid for modern browsers.