feat: Adopt the inline bootstrap loader

[chrisjshull]

https://developers.google.com/maps/documentation/javascript/load-maps-js-api#dynamic-library-import

[amuramoto]

This looks good, but we should retain support for the old way of loading, since a) removing it would be a major breaking change that requires a non-trivial code rewrite for users b) the old way is still officially valid and supported.

I think a good option would be to define a separate InlineLoaderOptions type, and have Loader accept both that and LoaderOptions. Then fork the logic based on the options object that Loader is instantiated with.

There's, of course, other ways to go about this, but we should make this change in a non-breaking way, so dropping the old loader entirely isn't an option.

[chrisjshull]

My goal was to make this a non-breaking change. Can you elaborate on the breakage you are seeing?

[amuramoto]

Ah I see, I had thought the libraries specified in LoaderOptions wouldn't be available until after importLibrary was called, because I wasn't aware of the libraries options prop of the inline loader that you are able to pass those through to.

One question since I haven't been able to test this manually yes - if a user is still using Loader.load(), is google.maps.Map available when the callback in invoked? I ask because when using the inline loader script outside this library, you need to specify libraries: ['maps'] in the script to have google.maps.Map available on load.

[chrisjshull]

One question since I haven't been able to test this manually yes - if a user is still using Loader.load(), is google.maps.Map available when the callback in invoked?

It should be...

I ask because when using the inline loader script outside this library, you need to specify libraries: ['maps'] in the script to have google.maps.Map available on load.

That's not how it's intended to function. Gave it a quick test: https://jsfiddle.net/q1xe8fs0/ - google.maps.Map is available as soon as any part of maps JS is loaded. Happy to investigate a sample that demonstrates otherwise.

Aside: are there plans to fix the e2e tests (#795) soon? Happy to wait for that if we want to increase confidence. So far my testing has just been the unit tests.

[amuramoto]

That's not how it's intended to function. Gave it a quick test: https://jsfiddle.net/q1xe8fs0/ - google.maps.Map is available as soon as any part of maps JS is loaded. Happy to investigate a sample that demonstrates otherwise.

If I remove await google.maps.importLibrary("core"); from that jsfiddle, then google.maps.Map is not available. AFAIK, this is the expected behavior from the inline API loader, but doesn't that then introduce a breaking change into this lib?

If I upgrade after this PR, I would have to explicitly add CoreLibrary or MapLibrary to the libraries prop in the bootstrap, then load one of those with importLibrary. This would be so the google.maps.Map is available by default at load time, which is the current behavior. (or it could be that CoreLibrary would be the right choice here? Maybe MapLibrary doesn't provide the same APIs on load as the old loader - I haven't done a comprehensive comparison between them.)

So existing code that looks like this:

const loader = new Loader({
  apiKey: "myKey",
  libraries: ["places"]
});

loader
  .load()
  .then((google) => {
    new google.maps.Map(document.getElementById("map"), mapOptions);
  });

would instead have to look like this:

const loader = new Loader({
  apiKey: "myKey",
  libraries: ["places", "maps"] // or "core"
});

loader
  .load()
  .then(async (google) => {
    await google.maps.importLibrary("maps"); // or core
    new google.maps.Map(document.getElementById("map"), mapOptions);
  });

Aside: are there plans to fix the e2e tests (#795) soon? Happy to wait for that if we want to increase confidence. So far my testing has just been the unit tests.

I'll try to take a look soon, since I'd like to get this PR merged. Since this is going to change the lib to only use the inline loader script under the hood, can you also take a look at /examples and update whatever is needed there?

[chrisjshull]

If I remove await google.maps.importLibrary("core"); from that jsfiddle, then google.maps.Map is not available. AFAIK, this is the expected behavior from the inline API loader, but doesn't that then introduce a breaking change into this lib?

That's because the inline bootstrap loader doesn't load anything until the first importLibrary call. That's why this PR includes a call to loader.importLibrary("core"); (really it could have been any library) inside setScript(), in order to maintain backwards compatibility.

If I upgrade after this PR, I would have to explicitly add CoreLibrary or MapLibrary to the libraries prop in the bootstrap, then load one of those with importLibrary. This would be so the google.maps.Map is available by default at load time, which is the current behavior. (or it could be that CoreLibrary would be the right choice here? Maybe MapLibrary doesn't provide the same APIs on load as the old loader - I haven't done a comprehensive comparison between them.)

So existing code that looks like this:

const loader = new Loader({
  apiKey: "myKey",
  libraries: ["places"]
});

loader
  .load()
  .then((google) => {
    new google.maps.Map(document.getElementById("map"), mapOptions);
  });

...

Tested manually, this still works.

Since this is going to change the lib to only use the inline loader script under the hood, can you also take a look at /examples and update whatever is needed there?

done

[amuramoto]

ok awesome - I didn't catch the importLibrary call in setScript. I think we can go ahead and merge this then. It's going to take a while longer to figure out the e2e tests, but all they do is load the map and set center.

[googlemaps-bot]

🎉 This PR is included in version 1.16.0 🎉

The release is available on:

• GitHub release
• npm package (@latest dist-tag)
• v1.16.0

Your semantic-release bot 📦🚀