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

docs(example): use externally loaded google maps api #513

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

mdogadailo
Copy link

This pull request adds support for using a Client ID in addition to the existing apiKey for the APIProvider component.

While Google recommends using an apiKey for most users, there are specific scenarios and legacy systems where an enterprise clientId is necessary or beneficial such as specific contractual or billing arrangements.

Component Changes:

  • Modified APIProviderProps to support either apiKey or clientId, but not both simultaneously.
  • Updated useGoogleMapsApiLoader to handle clientId in addition to apiKey.

Documentation Updates:

  • Updated docs/api-reference/components/api-provider.md to include information about the new clientId prop and its usage.

Testing:

  • Added a new test case to ensure clientId is correctly passed to GoogleMapsAPILoader.

@mdogadailo mdogadailo changed the title Add Support for Client ID as an Alternative to API Key for GMP Premium Plan feat(api-provider): Add Support for Client ID as an Alternative to API Key for GMP Premium Plan Sep 5, 2024
@usefulthink
Copy link
Collaborator

First off, thanks a lot for your work – I really appreciate you taking the time to jump into it. However, it would have been good to open a feature request to discuss this before starting the work.

While Google recommends using an apiKey for most users, there are specific scenarios and legacy systems where an enterprise clientId is necessary or beneficial such as specific contractual or billing arrangements.

Would you mind expanding on this a little bit?

As far as I know, the client-ids, along with the Premium Plans, have been deprecated several years ago and for a variety of reasons, its use isn't recommended anymore. All traces of it have also been removed from the documentation.
They still work, primarily to support legacy applications that were built with the clientId and aren't feasible to be updated anymore. New and updated applications should always use apiKeys, especially because they allow much more fine-grained permission-control.

If there are things that can be done with the clientId but can't be achieved with an apiKey, that would certainly be good to know and probably something to address in the issue tracker or via a support request.

I don't think we should be adding support for undocumented legacy features to this library when they offer little to no benefit over their modern alternatives..

@mdogadailo
Copy link
Author

@usefulthink you for your feedback on the pull request. I understand the concerns about adding legacy features that Google is no longer promoting, and I completely agree that switching to API Keys is preferable.

However, there is a significant issue related to what is known as the “Enterprise Adaptation Pace” which means they typically operate on significantly slower timelines than smaller companies, especially when it comes to updating or replacing large-scale IT systems.
Many legacy systems were built around specifications that included the use of Client IDs. These systems might require significant re-engineering to shift to API Keys. Moreover, a web application could be just a part of these systems or even be a part of a micro front-end within a larger web application.

There is also a tricky aspect related to Google’s announcement about standardizing:

"Google Maps Platform will be standardizing on using API keys to authenticate requests. While there is no current end-date for using client IDs, build any new implementations to use API keys rather than client IDs."

The use of “standardizing” instead of “deprecated,” and the absence of an end-date, suggest that client IDs will not be phased out in the immediate future.

By supporting Client IDs, we can provide a bridge for organizations planning a gradual transition to API Keys. This support can be crucial for large-scale applications where immediate transitions might lead to significant downtime or require substantial development effort.

Consider an old jQuery-only application that is migrating to React, not by completely rewriting the application, but by replacing features one by one (page by page). Because the old system still functions, it is sometimes decided that it is better to slowly add features to the old system rather than switch to a modular, reusable component architecture.
If the system is white-labeled, it can have more than just a few records for URL limits but potentially hundreds of them, managed by another application when a client is added or removed to company portfolio.

We can ensure that the documentation clearly advises new users against using Client IDs unless absolutely necessary. This approach balances legacy support with clear guidance towards newer technologies.

I believe supporting Client IDs, even temporarily, adds considerable value by increasing the library’s accessibility to organizations in the midst of transitions.

Additionally, for this library, supporting different authentication types does not require additional work. I also refrained from including examples using a “client ID” in the main documentation to avoid promoting this feature excessively. It is mentioned only in the readme, and this mention can be removed if it helps focus on promoting the use of API keys.

We can ensure that the documentation clearly advises new users against using Client IDs unless absolutely necessary. This approach balances legacy support with clear guidance towards newer approach.

@usefulthink
Copy link
Collaborator

As for legacy applications, that is exactly why the parameter is still working. The "standardizing" bit about apiKeys is exactly what has happened by now, and there is no longer any documentation about using clientIds (outside of the Premium Plan website, and that isn't liked to from anywhere I could find). So you could argue that it's already beyond deprecated and indeed removed.

I believe that when you begin working on a new feature, even within a legacy system, it is a good opportunity to transition from using client-ids to api-keys. That can be easily done by creating a new API key in the cloud-console without impacting other applications that continue to use client-ids. I don't see why this would be a huge undertaking.

In terms of doing a step-by-step migration as you mentioned: this library will not attempt to load the maps API if it has already been loaded externally (currently implemented by checking if the google.maps.importLibrary function exists at load time). So, if you want to create a react-component within an existing application that already loads the maps API with a clientId, this should work without a problem. The documentation could be a bit clearer on that, and we could add an extra prop like useExternallyLoadedMapsAPI for the APIProvider to make this clearer.

Improving the support for an externally loaded Maps API would be a more widely useful feature to focus on.

Additionally, for this library, supporting different authentication types does not require additional work.

That might actually be partly true, in that the code could just be there without anyone having to actively think about it.
But it adds complexity to the types, it adds code someone has to understand when working on it (this is where adding undocumented legacy features is problematic), and it could also lead to other issues down the line.

@mdogadailo
Copy link
Author

@usefulthink Thank you again for your detailed feedback and future direction. I agree with your points on the importance of transitioning to API keys for new developments within legacy systems. However, given the constraints that many organizations face which delay these updates, I have reconsidered the approach to better align with both the library’s goals and user needs.

Instead of modifying the library to directly support client IDs, as you suggested I have decided to focus on a partial solution that leverages the library’s existing ability to handle an externally loaded Maps API. Given that library already check for google.maps.importLibrary to avoid reloading the API, I realized that enhancing support for this scenario could be more beneficial and less intrusive.

I have updated my pull request to replace the original code with an example that demonstrates how to work with an externally loaded Maps API. This approach ensures that users who still rely on client IDs can seamlessly integrate their existing setups with new developments using this library.

@mdogadailo mdogadailo changed the title feat(api-provider): Add Support for Client ID as an Alternative to API Key for GMP Premium Plan docs(example): use externally loaded google maps api Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants