Use callback param instead of script onload

[tstirrat15]

Fixes #26

Motivation

See #26. This work is a bit speculative - I haven't verified that the failure mode that I described in #26 is actually what's happening, but this is the approach recommended by the google maps docs.

Changes

• Add onGoogleMapsLoad callback to window at the beginning of load
• Put callback logic into that callback
• Add callback parameter to params object, placing it before the destructuring so that a consumer could override it if they wanted to for whatever reason
• Only run script load callback if it's an error

Testing

Code review. See that tests pass. Tell me if there's another test that you'd like to see.

[codecov-io]

Codecov Report

Merging #27 into master will decrease coverage by 8.23%. The diff coverage is 63.63%.

@@            Coverage Diff             @@
##           master      #27      +/-   ##
==========================================
- Coverage   89.18%   80.95%   -8.24%     
==========================================
  Files           2        2              
  Lines          37       42       +5     
  Branches        7        8       +1     
==========================================
+ Hits           33       34       +1     
- Misses          3        6       +3     
- Partials        1        2       +1

Impacted Files	Coverage Δ
src/loadGoogleMapsSdk/index.js	80% <63.63%> (-12%)	:arrow_down:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 556e33e...af46fe8. Read the comment docs.

[tstirrat15]

Anecdotally, a similar change in our codebase resulted in a lower error rate.

[tstirrat15]

I'm not done with this, but I've pushed where I've gotten so far.

On master, the error behaviors aren't actually tested. The expect statements inside of the setTimeouts are never exercised - the test goes right past them to done() and calls the test good.

My guess for why the tests are considered "covered" is that setting up the callback involves reading through those lines, and the coverage counter considers them "covered" because of that.

With the auth error test, there's some weird behavior on the part of the Google Maps SDK. It runs the SDK load and the auth concurrently and asynchronously, and while the SDK loads in less than a second, the auth takes up to 6s to execute. This means that the onLoad callback is triggered before the auth error callback. I'm not sure if this means that there will need to be changes in the code that triggers the callback handed to loadGoogleMapsSdk.

With the network error test, the easiest way to get it to fail in the scope of the test is to change the URL that it's referencing. This is why I added a third argument to loadGoogleMapsSdk which defaults to the SDK URL. It's not intended to be a part of the public API, just a test convenience.

Finally, the network error test is still failing. It passes if you add test.only to that test and make it the only executed test in the module. This points to the module-level state (i.e. googleMaps and state) not getting reset between test runs, so the loadGoogleMapsSdk call short-circuits because state === LOADED.

I attempted to address this by adding the require in beforeEach, which should re-execute the module and reset the module's state before each test. However, jest runs tests in parallel by default, which means that there's a race condition, and the tests still fail. The simplest way to address this (short of a pretty major rework of loadGoogleMapsSdk that takes advantage of memoization or some other way of not relying on module scope) is to make the tests run in sequence. The docs for that are here, but I ran out of time to figure that out and implement it today. I'll see if I can return to this on Monday.

[tstirrat15]

... The fact that tests passed is concerning to me. I don't think they should have.

[EDIT]: Ahh, I didn't see that it's only showing the security test right now.

[cedricdelpoux]

Good investigation about this 👏🏻Running tests in sequence could be a good improvement

[cedricdelpoux]

@tstirrat15 What is the status of this ?

[tstirrat15]

Sorry, it very much fell through the cracks. I'll see if I can have another go at it in the next week.

[cedricdelpoux]

Closing because there is no update