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

Is deprecating load() and similar methods a good idea? #811

Closed
bjornharvold opened this issue Sep 11, 2023 · 4 comments
Closed

Is deprecating load() and similar methods a good idea? #811

bjornharvold opened this issue Sep 11, 2023 · 4 comments
Labels
triage me I really want to be triaged. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@bjornharvold
Copy link

bjornharvold commented Sep 11, 2023

Hi Team Google Maps!

I just came across your library and I wanted to have a closer look. Previously, we have our own Javascript loader that dynamically adds the google Maps JS script element to head and issues a loaded event in our reactive environment. The loaded event tells us we can safely create a new Map now that the google namespace is loaded and available.

In our Ngrx effect, it looks something like this:

import {Injectable} from '@angular/core';
import {Actions, createEffect, ofType} from '@ngrx/effects';
import {catchError, map, switchMap, withLatestFrom} from 'rxjs';
import {from, of} from 'rxjs';
import * as GoogleMapsActions from './google-maps.actions';
import {GoogleMapsLoaderService} from '../../infrastructure/google-maps-loader.service';
import {GoogleMapsFacade} from '../../application/google-maps.facade';

@Injectable()
export class GoogleMapsEffects {
  // load Google Maps library remotely
  load$ = createEffect(
    () =>
      this.actions$.pipe(
        ofType(GoogleMapsActions.loadGoogleMapsSDK),
        withLatestFrom(
          this.googleMapsFacade.loaded$,
          ({googleMapsAPIKey, libraries}, isLoaded) => {
            return {googleMapsAPIKey, libraries, isLoaded}
          }),
        switchMap(({googleMapsAPIKey, libraries, isLoaded}) => {
          if (googleMapsAPIKey == null || googleMapsAPIKey === '') {
            return of(GoogleMapsActions.googleMapsSDKUnavailable());
          }
          if (isLoaded === true) {
            return of(GoogleMapsActions.googleMapsSDKAlreadyLoaded());
          }

          return from(this.googleMapsLoaderService.load(googleMapsAPIKey, libraries)).pipe(
            map(() => GoogleMapsActions.googleMapsSDKLoaded({googleMapsAPIKey, libraries})),
            catchError(error => of(GoogleMapsActions.googleMapsSDKFailed({error: JSON.stringify(error)})))
          );
        })
      )
  );

  constructor(private readonly googleMapsLoaderService: GoogleMapsLoaderService,
              private readonly googleMapsFacade: GoogleMapsFacade,
              private readonly actions$: Actions
  ) {
  }
}

Notice that we do not do anything else at this point. We separate loading with instantiating. The event is all we need.

When looking at your loader, your documentation says to use methods that have all been deprecated and instead do something like this:

'loader.importLibraries('maps')`

Does this make any sense?

const loader = new Loader({apiKey: '', libraries: ['maps', 'drawing']});
loader.importLibrary('maps')

If we are using this npm module, it's probably because we want to load the Google Maps JS SDK, I would assume 'maps' gets loaded by default and libraries are all overlays and features on top of the actual map.

I would request that we keep the load() method around and not deprecate it [unless I am missing something obvious 🤔].

const loader = new Loader({apiKey: '', libraries: ['maps', 'drawing']});
loader.load()

Cheers,
Bjorn

@bjornharvold bjornharvold added triage me I really want to be triaged. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. labels Sep 11, 2023
@wangela
Copy link
Member

wangela commented Sep 11, 2023

If you would like to upvote the priority of this issue, please comment below or react on the original post above with 👍 so we can see what is popular when we triage.

@bjornharvold Thank you for opening this issue. 🙏
Please check out these other resources that might help you get to a resolution in the meantime:

This is an automated message, feel free to ignore.

@WatchTower001110
Copy link

const loader = new Loader({apiKey: '', libraries: ['maps', 'drawing']});
loader.importLibrary('maps')

@usefulthink
Copy link
Contributor

usefulthink commented Nov 5, 2023

The constructor and importLibrary functions are really all you need and they can be made to support any use-case that has previously been covered by the load method.

Instead of

const loader = new Loader({apiKey: '...', libraries: ['places']});
await loader.load();

you can now write

const loader = new Loader({apiKey: '...', libraries: ['places']});
await loader.importLibrary('maps');

for exactly the same behaviour.

In both cases - once the promise returned by load/importLibrary is fulfilled - the maps API is available using the well known global namespace google.maps.

In addition to that, the importLibrary function (which is also available as google.maps.importLibrary after loading) will return the library objects and allows for loading libraries on-demand.

So when different components in your application need access to different maps-libraries, e.g. Map and PlacesService, you can now do it like this:

// somewhere globally (note we're not specifying any libraries here)
const loader = new Loader({apiKey: '...'});

// ...in the component that renders the map
const {Map} = await loader.importLibrary('maps');
const map = new Map(mapDivEl, {center, zoom});

// ...in another component that needs the placesService
const placesLib = await loader.importLibrary('places');
const service = new placesLib.PlacesService();

This could help you save some requests for compnents that aren't actually needed everywhere in the application.

@bjornharvold
Copy link
Author

Works

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage me I really want to be triaged. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

No branches or pull requests

4 participants