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

Move google-auth-library to a peer dependency #1443

Merged
merged 1 commit into from
Jun 3, 2020

Conversation

murgatroid99
Copy link
Member

This fixes #1442. The code path that calls into google-auth-library can't actually be triggered from the current public APIs, so if someone doesn't have the peer dependency they'll be fine. Firebase depends on google-gax 1.x, which depends on google-auth-library 5.x, and other client libraries depend on google-gax 2.x, which depends on google-auth-library 6.x. The peer dependency will be satisfied in either case.

We also need to add the dev dependency because we need it at build time for the type definitions.

Once the xDS stuff is done (or we want to create a credentials.createGoogleDefault public API) and that code path can actually be triggered, we should switch back to a direct dependency on google-auth-library 5.x to ensure that we still support Node 8. Hopefully we can eventually drop Node 8 and switch the dependency to 6.x.

@samtstern
Copy link

@murgatroid99 really appreciate the quick fix!

@murgatroid99 murgatroid99 merged commit 5e9b852 into grpc:master Jun 3, 2020
@AviVahl
Copy link

AviVahl commented Jun 19, 2020

This change caused a peerDependency warning in every project using firebase. :/

Can someone clarify how is createGoogleDefaultCredentials even used? it's not referenced within repo, and is not even exported? should it even be in this package?

@murgatroid99
Copy link
Member Author

It's an internal part of an in-progress feature (the "xDS stuff" I mentioned in the description), and it might become a public API in the future. As mentioned, once that feature is complete, this will switch to a direct dependency on google-auth-library. Until then, I think having this peer dependency is probably the least-bad option.

@AviVahl
Copy link

AviVahl commented Jun 19, 2020

I can't say I agree with you. The current approach is adding a warning to build logs just to keep supporting an EOL version of Node. Droping Node 8 (via a major release) is probably the "least bad" option in this case.

@murgatroid99
Copy link
Member Author

Dropping Node 8 is not an option. We need to support some platforms that are still on Node 8.

The viable options that I can see here are:

  • Have the peer dependency as it currently exists. The cost is a one-line warning in many build logs.
  • Add a direct dependency on google-auth-library version 5.x. The cost is a spurious package in most people's dependency trees.
  • Have no dependency. The cost is a disconnect between require calls in the code and the dependency list, though that require call is currently unreachable from the public API.

@AviVahl
Copy link

AviVahl commented Jun 19, 2020

I have ~10 warnings already in my log. Why? Because 10 different people, of very common libraries and tools out there, decided that a "one-line" warning is acceptable. For me, it feels like a failure to see the rest of the ecosystem. :(

The third option you've mentioned sounds much better than the first, considering this is an in-progress feature not part of the public API.

@murgatroid99
Copy link
Member Author

You convinced me. Version 1.1.2 has deleted that peer dependency.

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.

Dependency on google-auth-library 6 breaks Node 8 support
4 participants