feat: load the google maps api dynamically #193
Conversation
|
@sandydoo Please review this one too 🙏 |
|
@sandydoo Let me know any feedback you have on this one 🙏 |
|
I need some time to read up on this loading method to understand the implications of this switch. I do not yet know if we should support both methods or release a breaking change. The new method allows us to async load the other libraries. Maybe we can take advantage of this instead of loading everything at once in a loop. Note To anyone reading this, the Google Maps "warning" in the console is not a critical issue. It's just a new, preferred loading method. The current method is very much supported. |
There was a problem hiding this comment.
Thanks for looking into this! The two major issues I see with this are:
- The sequential library loading.
- The breaking changes to
buildGoogleMapsUrl.
2 is the bigger issue for me. It's a bit more work on our end, but I would prefer to release a minor version first, then drop the old loader in a major bump.
| for (let library of libraries) { | ||
| await google.maps.importLibrary(library); | ||
| } |
There was a problem hiding this comment.
This should at least use Promise.all instead of loading everything sequentially.
This is good enough for a first pass. Once the old loader is gone we can migrate everything to take advantage of the dynamic loading capabilities.
There was a problem hiding this comment.
Also, we need to mention in the docs that libraries must contain map.
| * allows you to use external data when building the URL. For example, you | ||
| * could fetch the database record for the current user for localisation | ||
| * purposes. | ||
| */ | ||
| buildGoogleMapsUrl(config) { |
There was a problem hiding this comment.
Removing a public function like this is a breaking change. It would be great if this continued to work as before. We can detect if this was overridden and provide a dev-only deprecation notice.
There was a problem hiding this comment.
@sandydoo if it's useful, the one time I needed to adjust things around config (6 years ago in an app that is no longer online) I did it here. Can't remember very clearly what was needed - I think it was swap in custom info in a whitelabel scenario), but thought I'd speak up.
In that particular case, if I remember right, the ability to return a promise was invaluable
| s.textContent = | ||
| `(g=>{var h,a,k,p="The Google Maps JavaScript API",c="google",l="importLibrary",q="__ib__",m=document,b=window;b=b[c]||(b[c]={});var d=b.maps||(b.maps={}),r=new Set,e=new URLSearchParams,u=()=>h||(h=new Promise(async(f,n)=>{await (a=m.createElement("script"));e.set("libraries",[...r]+"");for(k in g)e.set(k.replace(/[A-Z]/g,t=>"_"+t[0].toLowerCase()),g[k]);e.set("callback",c+".maps."+q);a.src=\`https://maps.\${c}apis.com/maps/api/js?\`+e;d[q]=f;a.onerror=()=>h=n(Error(p+" could not load."));a.nonce=m.querySelector("script[nonce]")?.nonce||"";m.head.append(a)}));d[l]?console.warn(p+" only loads once. Ignoring:",g):d[l]=(f,...n)=>r.add(f)&&u().then(()=>d[l](f,...n))})({ | ||
| key: "${key}",` + | ||
| versionString + | ||
| ` | ||
| ` + | ||
| channelString + | ||
| ` | ||
| ` + | ||
| regionString + | ||
| ` | ||
| ` + | ||
| languageString + | ||
| ` | ||
| ` + | ||
| mapIdsString + | ||
| ` | ||
| // Add other bootstrap parameters as needed, using camel case. | ||
| }); | ||
| initGoogleMap(); | ||
| `; |
There was a problem hiding this comment.
We can skip going through a script tag entirely and run this snippet as is.
| @@ -312,82 +311,6 @@ module.exports = { | |||
| }); | |||
| }, | |||
|
|
|||
| buildGoogleMapsUrl(config = {}) { | |||
There was a problem hiding this comment.
Same as before, this should continue working.
|
Alternatively, we could release a breaking change and check whether |
Loads the maps api dynamically as is the recommended way.