Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Proposal for a 2.0 version of this library #842

Open
usefulthink opened this issue Apr 5, 2024 · 4 comments
Open

[RFC] Proposal for a 2.0 version of this library #842

usefulthink opened this issue Apr 5, 2024 · 4 comments
Labels
type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@usefulthink
Copy link
Contributor

usefulthink commented Apr 5, 2024

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 usefulthink added the type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. label Apr 5, 2024
@usefulthink
Copy link
Contributor Author

/cc @wangela @chrisjshull

@chrisjshull
Copy link
Contributor

+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.

@usefulthink
Copy link
Contributor Author

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?

@chrisjshull
Copy link
Contributor

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.

@wangela wangela pinned this issue Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

No branches or pull requests

2 participants