Skip to content

Allow RemoteConfig to auto-generate typings, separate internal vs external APIs #984

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 8 commits into from
Aug 11, 2020

Conversation

MathBunny
Copy link
Contributor

Context

The goal is to bridge the gap between today's Admin SDK (manually curated d.ts files, nested namespaces) and a modularized SDK that has auto-generated typings.

This is the seventh milestone from: go/firebase-node-auto-typing.

Goals for this PR

  1. Allow to auto-generated typings for RC
  2. Split the internal and external APIs
  3. Still use the previous manually curated typings
  4. Be a non-breaking change

Prerequisites

None.

Test Plan

  1. Unit tests:

npm test

  1. Integration tests:

export TYPE_GENERATION_MODE=nonauto && npm run integration && export TYPE_GENERATION_MODE=auto && npm run integration

  1. Sanity tests:

export TYPE_GENERATION_MODE=nonauto && npm pack && ./.github/scripts/verify_package.sh firebase-* && export TYPE_GENERATION_MODE=auto && npm pack && ./.github/scripts/verify_package.sh firebase-*

@MathBunny MathBunny requested a review from hiranya911 August 7, 2020 18:59
@MathBunny MathBunny requested a review from lahirumaramba August 7, 2020 18:59
@MathBunny MathBunny marked this pull request as ready for review August 7, 2020 18:59
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. Just one suggestion from me. Please wait for @lahirumaramba to review.

RemoteConfigTemplate,
TagColor,
ListVersionsResult,
Version,
} from '../../../src/remote-config/remote-config-api-client';
import { FirebaseRemoteConfigError } from '../../../src/remote-config/remote-config-utils';
import { RemoteConfigApiClient } from '../../../src/remote-config/remote-config-api-client-internal';
import { FirebaseRemoteConfigError } from '../../../src/remote-config/remote-config-utils-internal';
Copy link
Contributor

Choose a reason for hiding this comment

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

Just remote-config-internal. Or just move into the remote-config-api-client-internal module. *-utils-internal is an unusual name.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah. remote-config-utils only contains FirebaseRemoteConfigError and error codes. We should be able to move it into remote-config-api-client-internal

@hiranya911 hiranya911 removed their assignment Aug 10, 2020
Copy link
Member

@lahirumaramba lahirumaramba left a comment

Choose a reason for hiding this comment

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

LGTM!
Please check Hiranya's comment. Thank you!

RemoteConfigTemplate,
TagColor,
ListVersionsResult,
Version,
} from '../../../src/remote-config/remote-config-api-client';
import { FirebaseRemoteConfigError } from '../../../src/remote-config/remote-config-utils';
import { RemoteConfigApiClient } from '../../../src/remote-config/remote-config-api-client-internal';
import { FirebaseRemoteConfigError } from '../../../src/remote-config/remote-config-utils-internal';
Copy link
Member

Choose a reason for hiding this comment

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

Yeah. remote-config-utils only contains FirebaseRemoteConfigError and error codes. We should be able to move it into remote-config-api-client-internal

Copy link
Member

@lahirumaramba lahirumaramba left a comment

Choose a reason for hiding this comment

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

Please check the comment on remote-config-utils... other than that everything else look good!

@MathBunny
Copy link
Contributor Author

Thanks for the feedback, PTAL :)

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.

LGTM

@hiranya911 hiranya911 removed their assignment Aug 11, 2020
Copy link
Member

@lahirumaramba lahirumaramba left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

@MathBunny MathBunny merged commit 875d865 into master Aug 11, 2020
@MathBunny MathBunny deleted the hlazu-modularization-rc branch August 12, 2020 18:28
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.

3 participants