js-api-loader icon indicating copy to clipboard operation
js-api-loader copied to clipboard

[RFC] Proposal for a 2.0 version of this library

Open usefulthink opened this issue 1 year ago • 4 comments

I think, given the very limited scope of what this library is supposed to do, it can be significantly simplified for a 2.0 version when effectively rewriting it around the ideas of the dynamic library loading.

The library as it is now seems unnecessarily complicated.

  • the exported Loader class is a de-facto singleton - without being implemented as one. This makes it needlessly complicated to use.
  • for a lot of tasks, there are several different ways to achieve the same result, where we can just provide one
  • there are lots of deprecated functions and other details that are outdated or not really needed (e.g. functions to remove the API, which – with newer versions – can't be done completely anyway)

Proposed API

The proposed API exposes just two main parts of functionality: a (static) method to configure the options and the importLibrary function (which is exported independently for convenience).

import ApiLoader from "@googlemaps/js-api-loader";

// initial configuration
ApiLoader.setOptions({ apiKey: "...", version: "weekly" });

// loading a library
const { Map } = ApiLoader.importLibrary("maps");
// anywhere else in the application the shorthand-version can be used as well
import { importLibrary } from "@googlemaps/js-api-loader";

// `importLibrary` is an alias for `ApiLoader.importLibrary`.
const { Map } = await importLibrary("maps");

To get status-information and error messages, the ApiLoader can implement the EventTarget interface (add/removeEventListener):

// status could be initialized, loading, loaded, error, or auth-failure
ApiLoader.addEventListener("status-changed", (ev) => {
  console.log("new status:", ev.status);
});

ApiLoader.addEventListener("error", (ev) => {});

Notes on internal behavior

  • the ApiLoader doesn't do anything (except for storing the options) until the importLibrary function is called for the first time. This allows users to configure the loader in a central place of their application even if the maps API isn't needed on most pages

  • Once the importLibrary function is called, the options are frozen and attempts to modify them will throw an Error (or maybe just log an error-message to the console?)

  • the first call to importLibrary initiates the bootstrapping, once the maps API is loaded, importLibrary will directly forward to the google.maps.importLibrary function.

  • if an attempt to load the API fails, the loader will resolve all pending importLibrary promises with an error and will retry loading with the next importLibrary call.

  • if the library was already loaded by other means than the ApiLoader, a warning is logged to the console

usefulthink avatar Apr 05 '24 17:04 usefulthink

/cc @wangela @chrisjshull

usefulthink avatar Apr 05 '24 17:04 usefulthink

+1 At a high level this lines up nearly exactly with the ideas that formed the dynamic library loader, which is great. Aside from more minor things (like naming), I’m curious to hear more about the need for eventing (vs just using the promises from importLibrary). And internally my request would be to actually have a copy of https://developers.google.com/maps/documentation/javascript/load-maps-js-api#dynamic-library-import as the internal implimentation detail, to make it easy to update here when we update there.

chrisjshull avatar Apr 05 '24 17:04 chrisjshull

I’m curious to hear more about the need for eventing

The one thing where events are useful is when including the handling of gm_authFailure into the loader, since that is an error that happens after the promises already resolved (at least that was the reason I added them internally to the loader we use in @vis.gl/react-google-maps). I looked through all other places where we use the loading-status and in fact we could write all of them in a way that doesn't need the status.

And internally my request would be to actually have a copy of https://developers.google.com/maps/documentation/javascript/load-maps-js-api#dynamic-library-import as the internal implementation detail, to make it easy to update here when we update there.

I see your point, that would be pretty much exactly what @googlemaps/extended-component-library is doing. I'm not too strongly opposed to that but I've had a lot of situations in the past where being able to step-debug through third-party code helped me understand errors and wrong assumptions in my own code. That's not so much fun with obfuscated and minified code (I spent quite some time figuring out what the google maps API is doing and why it's not what I expected). Is there maybe a "sourcecode" version of the minified loader code or is that handcrafted?

usefulthink avatar Apr 05 '24 19:04 usefulthink

The one thing where events are useful is when including the handling of gm_authFailure into the loader

Our newer APIs tend to have their own error reporting mechanisms, including auth (instead using the global handler). These days, auth failure in one API doesn’t mean auth failure globally. Let’s discuss fleshing that out instead of tying into gm_authFailure.

Is there maybe a "sourcecode" version of the minified loader code or is that handcrafted?

Internally, yes. I can look into publishing, but no promises.

chrisjshull avatar Apr 13 '24 19:04 chrisjshull