Google Maps JavaScript API has issues with Ractive's Promise polyfill

monoblaine commented 5 years ago

Description:

Google Maps JavaScript API has issues with Ractive since v3.33.

In a browser that does not support Promise (let's just say IE) Ractive's Promise polyfill is automatically used and the map loads with "Sorry, we have no imagery here" messages all over the place. Also, overlays (markers, polylines, etc.) are misplaced (slightly towards north or maybe just upwards). If we use another Promise polyfill (like https://github.com/stefanpenner/es6-promise) the problem goes away. (The Maps API also has its own polyfill, btw)

So after figuring out that our Promise polyfill was to blame, I've tried to find what was wrong with it. I've executed the test suite in https://github.com/promises-aplus/promises-tests but all the tests pass, so apparently no issues with the spec.

There are differences between the native Promise and the Ractive's Promise: Native one has functions defined in Promise.prototype (then, catch and finally) but Ractive's Promise constructor returns a POJO with then and catch functions and no functions are defined in Promise.prototype. However I'm not sure if this is really the reason to our problem.

Workaround

Just include another Promise polyfill before Ractive's.

What should be done?

We should either find what's wrong with Ractive's polyfill and fix it or just remove the Ractive's polyfill and use an external one.

Versions affected:

All

Platforms affected:

Browsers that lack support for Promises.

Reproduction:

https://jsfiddle.net/monoblaine/4hpdc7wz/8/ (Could be viewed in any browser)

The line <script>window.Promise = undefined;</script> is to bypass native Promise.

The following one is the expected output:

https://jsfiddle.net/monoblaine/4hpdc7wz/11/

evs-chris commented 5 years ago

This is really weird, and I'm not entirely sure how to approach it. I suppose we could swap out our promise implementation. What's happening really makes no sense though, as we're conforming to the spec. Is there some newer spec available that maps is expecting to be available?

fskreuz commented 5 years ago

Looks like Ractive's Promise might be the weird one, considering that the native and the other polyfill works. 🤔

For the curious bunch:

It might be worth looking into how the polyfill's implementation differ's from Ractive's.
It might also be worth running Ractive's Promise against the other polyfill's tests (the polyfill's tests might have some stuff not in the spec tests).

For a quick fix:

For the user, just use another polyfill (override the one that comes with Ractive).
For Ractive, maybe shamelessly replace our implementation with the polyfill's? (it's open-source after all) 😂

monoblaine commented 5 years ago

I've tried debugging through the minified, ugly and cryptic maps api source code for hours to no avail.

I hope to dig more into it when I've got some free time because I'm really curious, too.

Note to myself:

Investigate and compare other polyfill libraries
Try adding POJO functions (then and catch) to Promise prototype. (Although I could swear I tried this I'm not convinced enough)
Is it because Ractive's Promise lacks Promise.prototype.finally?

evs-chris commented 5 years ago

It might be the finally implementation. I'm going to poke at that a little and see what happens.

I looked at the linked es6 polyfill and it does quite a bit differently. We have a sort of minimal implementation that's fairly straightforward. I can't imagine a classier implementation having any functional differences though.

monoblaine commented 5 years ago

I think I've become obsessed with this. But I cannot still find the problem.

Here are some updates:

The fact that we're not using Promise.prototype is indeed not related with this problem.
It's not about the missing finally, either. (I've searched through the maps api source code: There are no finallys.)

I've come to realize that the tests in https://github.com/promises-aplus/promises-tests are insufficient. I've tried the tests in https://github.com/stefanpenner/es6-promise and bingo: We've got errors. I wish I could attach the the test results or create some repo but the way I executed the tests are so stupid and shameful: I've copied the transpiled Promise polyfill code from Ractive's js output file and pasted it into the appropriate place in browserify.js 😳. Anyway, the test results indicate that there's something wrong with our promise resolution function.

However, trying to fix this problem feels so pointless and a waste of time as Promises are widely supported today. So I hope I'll get over it and no longer investigate this.

One last thing:

https://github.com/ractivejs/ractive/blob/aac51d7c967be7f8e7f3559d3888be1501c89d2d/src/polyfills/Promise.js#L132

This might be related because that statetement can never be true as new Promise(...) returns a simple object. It does not return an instance of Promise.

fskreuz commented 5 years ago

This issue does bring back to life the idea of "bring your own polyfills" 🤔 (i.e. Ractive to shed polyfils, use es5-shim/es6-shim/core-js instead). Bugs like this was what I had in mind back then, as well as removing the responsibility of shimming from Ractive.

my 2c

evs-chris commented 5 years ago

Ok, this is super weird. Apparently the maps api depends on the same promise being returned on Promise.resolve(somePromise) somehow. Adding a one-line check for early return to Promise.resolve and Promise.reject seems to clear the issue up. The implementations for race and finally are also missing and relatively simple, so I'll probably go ahead and stick them in too.

There are a bunch of weird corner-case behaviors in Promise api that we're not compliant with, mostly having to do with throwing errors in really weird situations, but I think those can be ignored in practice. The passthrough on resolving and rejecting directly with a promise is in the spec, but I don't see how it could really affect anything :man_shrugging:.

I imagine somewhere around 2.0 we'll drop all of the polyfills. It's a delicate balance between low barrier to entry and weight (both maintenance and payload), and I think the balance is finally getting close to tipping.

monoblaine commented 5 years ago

Now I can die in peace 😄 Thank you so much @evs-chris !

ractivejs / ractive