feat: initial draft-implementation for the 2.0 API #895
Conversation
usefulthink
changed the title
| * official dynamic library import script: | ||
| * https://developers.google.com/maps/documentation/javascript/load-maps-js-api | ||
| * | ||
| * Formatting etc: |
There was a problem hiding this comment.
this is a really good job of de-minimizing!
but my concern with respect to these changes is keeping them in sync over time. preferably we could set up a pipeline from the google-internal version of this to GitHub that didn't require manual intervention.
we're looking into this internally
There was a problem hiding this comment.
I'm also seeing more changes than just these (e.g. throwing instead of logging particular errors) - I think we should be careful to adhere to the same design choices though.
There was a problem hiding this comment.
Fair point. I don't think there will be situations where this loader and the one in the documentation have to handle things differently. I don't think there will be a lot of reasons for this to change at all (assuming the TrustedTypesPolicy could be implemented upstream).
There might also be a few details that slipped through, I remember there being some things that looked strange to me in the minified code... The custom error here is just part of an attempt to handle network-errors separately from other error-conditions.
| */ | ||
| export function isLibraryImported(libraryName: ApiLibraryName): boolean { | ||
| return libraryName in libraries_; | ||
| } |
There was a problem hiding this comment.
on getImportedLibrary and isLibraryImported, I'd prefer to only expose these two if they become available on google.maps. And for that I'd like to discuss some concrete use cases for them, where they provide value.
(There's also several ways these two functions, as-is, could return the incorrect answer.)
There was a problem hiding this comment.
This all comes down to the contagious nature of async functions.
To use it as intended, the function from which importLibrary is called must be an async function itself.
This isn't always possible: Constructors can't be async, Frameworks like React don't allow async functions in some places, and using async functions or Promises can require compromises in architecture that we don't want to make.
I will quite often resort to using the global google.maps namespaces instead of the libraries, just because I don't want to deal with the async/await everywhere. As an example, I want to create a LatLngBounds object while updating some data for a map. Using const {LatLngBounds} = await google.maps.importLibrary('core'); would require the entire function to be async, but it's only the initial loading the maps library is actually async.
I'm not really happy with the naming, but adding something like those two functions to the Maps API proper would be really appreciated.
There was a problem hiding this comment.
I hear that, but on the other hand I haven't seen any reports of these troubles to our public issue tracker (https://issuetracker.google.com/issues?q=status:open%20componentid:188853&s=created_time:desc). That would be a good place to start (and optimally, get even more concrete in the use case). More reports/votes helps guide prioritization.
I'll also note that in a lot of cases we have "Literal" interface types, which can help with LatLngBounds types of things (i.e. LatLngBoundsLiteral) - unless particular LatLngBounds methods are desired.
| throw new Error( | ||
| "options cannot be modified after the API has been loaded." | ||
| ); | ||
| } |
There was a problem hiding this comment.
bootstrap is actually intended to gracefully ignore attempts to re-bootstrap (instead of throwing)
(see later note about keeping to similar design choices)
There was a problem hiding this comment.
We can surely do that as well.
src/2.0/loader.ts
Outdated
| }; | ||
| } | ||
|
|
||
| const getTrustedScriptUrl: (url: string) => string | TrustedScriptURL = (() => { |
There was a problem hiding this comment.
... I can see this being a layer we add here though
src/2.0/loader.ts
Outdated
| solutionChannel?: string; | ||
| }; | ||
|
|
||
| export class ApiLoadingError extends Error {} |
There was a problem hiding this comment.
s/ApiLoadingError/APILoadingError/
s/ApiOptions/APIOptions/
src/2.0/2.0.md
Outdated
|
|
||
| // when guarded by isLibraryImported, it's guaranteed to not throw | ||
| if (isLibraryImported("maps")) { | ||
| const { Map } = importLibrarySync("maps"); |
There was a problem hiding this comment.
should this be getImportedLibrary?
src/2.0/index.ts
Outdated
| // once proper typings are implemented in @types/google.maps | ||
| // (https://github.com/googlemaps/js-types/issues/95) | ||
|
|
||
| interface ApiLibraryMap { |
There was a problem hiding this comment.
I've noted a few, but throughout I'd aim for consistent casing of acronyms (either all-lower or all-upper). This follows the example of built-ins like JSON and createScriptURL (we will ignore XMLHttpRequest as being from the "bad ol' days" :P )
src/2.0/loader.ts
Outdated
| language?: string; | ||
| region?: string; | ||
| authReferrerPolicy?: string; | ||
| mapIds?: string; |
src/2.0/loader.ts
Outdated
| export type ApiOptions = { | ||
| key?: string; | ||
| v?: string; | ||
| libraries?: string; |
| } | ||
|
|
||
| return libraries_[libraryName] as TLibrary; | ||
| } |
There was a problem hiding this comment.
while I like adding better typing to importLibrary, that seems like something that could be better to fix directly in the google.maps typings.
also, from a usage perspective I'm wondering this should simply invoke the global google.maps.importLibrary - if there are two libraries that pin different (though compatible) versions of the bootstrap loader that might cause bootstrap to be triggered twice?
There was a problem hiding this comment.
Regarding the typings, there's already everything in place for @types/google.maps being updated at one point. I just didn't want to ship the v2-version with the incomplete types currently in @types/google.maps.
In situations where multiple versions of this package are somehow bundled in an Application, it's going to cause a problem, since the isBootstrapped_ variable is present twice as well (this will cause an error being thrown here: https://github.com/googlemaps/js-api-loader/pull/895/files#diff-debc445476274f687d8289e92416ea861546bc49990b42dc316f2a85faa5872eR36).
So it would probably be a better Idea to use presence of the google.maps.importLibrary function instead of the isBootstrapped_ variable.
| throw new Error( | ||
| "options cannot be modified after the API has been loaded." | ||
| ); | ||
| } |
There was a problem hiding this comment.
We can surely do that as well.
| } | ||
|
|
||
| return libraries_[libraryName] as TLibrary; | ||
| } |
There was a problem hiding this comment.
Regarding the typings, there's already everything in place for @types/google.maps being updated at one point. I just didn't want to ship the v2-version with the incomplete types currently in @types/google.maps.
In situations where multiple versions of this package are somehow bundled in an Application, it's going to cause a problem, since the isBootstrapped_ variable is present twice as well (this will cause an error being thrown here: https://github.com/googlemaps/js-api-loader/pull/895/files#diff-debc445476274f687d8289e92416ea861546bc49990b42dc316f2a85faa5872eR36).
So it would probably be a better Idea to use presence of the google.maps.importLibrary function instead of the isBootstrapped_ variable.
| */ | ||
| export function isLibraryImported(libraryName: ApiLibraryName): boolean { | ||
| return libraryName in libraries_; | ||
| } |
There was a problem hiding this comment.
This all comes down to the contagious nature of async functions.
To use it as intended, the function from which importLibrary is called must be an async function itself.
This isn't always possible: Constructors can't be async, Frameworks like React don't allow async functions in some places, and using async functions or Promises can require compromises in architecture that we don't want to make.
I will quite often resort to using the global google.maps namespaces instead of the libraries, just because I don't want to deal with the async/await everywhere. As an example, I want to create a LatLngBounds object while updating some data for a map. Using const {LatLngBounds} = await google.maps.importLibrary('core'); would require the entire function to be async, but it's only the initial loading the maps library is actually async.
I'm not really happy with the naming, but adding something like those two functions to the Maps API proper would be really appreciated.
| * official dynamic library import script: | ||
| * https://developers.google.com/maps/documentation/javascript/load-maps-js-api | ||
| * | ||
| * Formatting etc: |
There was a problem hiding this comment.
Fair point. I don't think there will be situations where this loader and the one in the documentation have to handle things differently. I don't think there will be a lot of reasons for this to change at all (assuming the TrustedTypesPolicy could be implemented upstream).
There might also be a few details that slipped through, I remember there being some things that looked strange to me in the minified code... The custom error here is just part of an attempt to handle network-errors separately from other error-conditions.
This is a first draft for what the 2.0 version of the js-api-loader might look like.
Since this is a rewrite of the old loader, I did all the work in separate files - those would replace the existing implementation in the real PR for the 2.0 version.
One thing I haven't yet figured out is how to correctly do error-handling for
ApiLoadingError: Such an error – likely related to network conditions, i.e. request timeout – should result in the library being able to start over as if the first load didn't happen.I'll give that more thought when working on the unit-tests for the new implementation.