refactor!: Remove Transporter
#1937
Conversation
There was a problem hiding this comment.
In general, a few notes:
- The issues linked here don't make it immediately clear to me that we need to remove
transporter. Can you help explain how you came to this solution? - It seems like this is just removing
transporter. Since that is the case, can we add a warning to thetransporterproperty, and confirm it with a test? If it really is refactoring, can we have test parity with the previoustransporterstest file? - Can you provide instructions for how downstream libraries will need to handle this change?
- Can you link any relevant PRs that explain how gaxios can provide this functionality? It seems like we're now just relying on gaxios for
transporterand I couldn't immediately tell which commits where relevant here
src/auth/authclient.ts
Outdated
| */ | ||
| transporter?: Gaxios | Transporter; | ||
| gaxios?: Gaxios; |
There was a problem hiding this comment.
How will these changes affect code here: https://github.com/googleapis/google-api-nodejs-client/blob/f4848c7295f887ae311c93967ee4723e336f263c/test/test.ts?
There was a problem hiding this comment.
I'm getting a 404 and I'm not seeing it on the main branch
There was a problem hiding this comment.
There was a problem hiding this comment.
Np, those aren't the same transporters referenced in this PR.
test/test.authclient.ts
Outdated
| @@ -38,4 +39,31 @@ describe('AuthClient', () => { | |||
| assert.equal(authClient[camelCased], value); | |||
| } | |||
| }); | |||
|
|
|||
| it('should allow disabling of the default interceptor', () => { | |||
There was a problem hiding this comment.
Ultimately, I'm a bit confused about this change: are we replacing functionality or removing it entirely? If replacing, it would be nice to see more test coverage (something at parity with confirming transporters functionality). If the latter, then I think maybe it would be best to add some type of warning for users attempting to use the Transporter property.
That's fair, updated the description with particular incidences and background.
Keeping transporter adds complexity to other future features, especially auth metrics, which would require more context from the auth instance itself. A few tests currently covered the existing resulting behavior from Transporter. Examples:
I'll carry over a few more explicit tests.
Yep, I'm preparing a general migration guide for this auth release. I haven't seen our top libraries use custom transporters, so the migration shouldn't change much.
As As for the auth-centric components, namely |
|
Ok, sgtm. Thanks for the explanation. Would it be possible to keep this test file, and replace with the Gaxios |
Yep, modernized and migrated the tests over into AuthClient's test file |
…js into remove-transporter
Description
The
Transporterinterface has been a pain-point for customers for years. To highlight a few:Additionally, this was the primary blocker for Firebase-Admin's adoption for this library:
As
Transporteris essentially a wrapper aroundGaxioswith a few auth-centric methods we can remove this layer of complexity to greatly improve UX.Testing
As we're removing
Transporterunit test coverage has improved with this change.Additionally, the current integration tests in pass with minimal change.
Impact
With this change we're optimizing the configuration and request flow to reduce complexity:
Additionally, this will unblock:
gaxiosv6 (as it would streamline the PR and make it significantly smaller given we do not have)fetchAPI Support in Auth (viagaxiosv6)Additional Information
In a separate PR I will draft a migration guide for customers migrating from v9 to v10.
As a TL;DR for migrating:
TransporteruseGaxiosinstead, which is conveniently re-exported by this librarytransporterconfig no changes are necessary🦕