Skip to content

Allow Credential to auto-generate typings, separate internal vs external APIs #1012

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

Merged
merged 6 commits into from
Aug 26, 2020

Conversation

MathBunny
Copy link
Contributor

Allows Credential to auto-generate typings and separate internal vs external APIs.

Follow-up to #1011.

@MathBunny MathBunny requested a review from hiranya911 August 25, 2020 22:41
@MathBunny MathBunny marked this pull request as ready for review August 25, 2020 22:41
Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. One suggestion on avoiding the cyclic dependency.

@hiranya911 hiranya911 assigned MathBunny and unassigned hiranya911 Aug 25, 2020
@MathBunny MathBunny assigned hiranya911 and unassigned MathBunny Aug 26, 2020
@MathBunny MathBunny requested a review from hiranya911 August 26, 2020 17:26
Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Let's rename the new module to something more appropriate.

@@ -19,7 +19,7 @@ import fs = require('fs');
import os = require('os');
import path = require('path');

import { GoogleOAuthAccessToken, Credential } from './credential';
import { GoogleOAuthAccessToken, Credential } from './google-oauth-access-token';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets call it credential-interfaces.ts since it has more than just the access token type.

@@ -15,6 +15,7 @@
*/

import * as credentialApi from './credential';
import * as googleOauthAccessTokenApi from './google-oauth-access-token';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GoogleOAuthAccessToken interface should also be exported. But looks like it's part of the admin namespace today. So perhaps we should do it later once we start refactoring the top-level admin namespace.

@hiranya911 hiranya911 assigned MathBunny and unassigned hiranya911 Aug 26, 2020
@MathBunny MathBunny requested a review from hiranya911 August 26, 2020 20:43
@MathBunny MathBunny assigned hiranya911 and unassigned MathBunny Aug 26, 2020
Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. LGTM 👍

@hiranya911 hiranya911 assigned MathBunny and unassigned hiranya911 Aug 26, 2020
@MathBunny MathBunny merged commit 1b1dbb7 into master Aug 26, 2020
@hiranya911 hiranya911 deleted the hlazu-modularizing-credential branch May 18, 2021 18:46
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