Skip to content

Conversation

lahirumaramba
Copy link
Member

  • Add interfaces to index.d.ts

Note: Integration tests will be added in an upcoming PR

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.

Add unit tests please.

});

it('should return a cached version of Messaging on subsequent calls', () => {
it('should return a cached version of Storage on subsequent calls', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed a typo. I can move this to a separate PR if we prefer that :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks 👍

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 👍

Over to @egilmorez for API doc review.

@hiranya911 hiranya911 assigned egilmorez and unassigned hiranya911 Mar 24, 2020
@hiranya911 hiranya911 requested a review from egilmorez March 24, 2020 19:25
src/index.d.ts Outdated
*
* @example
* ```javascript
* // Get the RemoteConfig service for the default app
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest backticks if we mean this as a literal, or just "Remote Config" if we mean to refer to the product name.

src/index.d.ts Outdated
*
* @example
* ```javascript
* // Get the RemoteConfig service for a given app
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest backticks if we mean this as a literal, or just "Remote Config" if we mean to refer to the product name.

src/index.d.ts Outdated
* ```
*
* @param app Optional app whose `RemoteConfig` service to
* return. If not provided, the default `RemoteConfig` service will be returned.
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this makes it sound even worse, but I don't think we can use "whose" correctly that way. Suggest: "Optional app for which to return the RemoteConfig service."

src/index.d.ts Outdated
* ```
*
* @param app Optional app whose `RemoteConfig` service to
* return. If not provided, the default `RemoteConfig` service will be returned.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest "is returned."

src/index.d.ts Outdated
* return. If not provided, the default `RemoteConfig` service will be returned.
*
* @return The default `RemoteConfig` service if no
* app is provided or the `RemoteConfig` service associated with the provided
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest comma: "provided, or"

src/index.d.ts Outdated

/**
* Interface representing a Remote Config parameter.
* At minimum, a `defaultValue` or a `conditionalValues` entry should be present for the
Copy link
Contributor

Choose a reason for hiding this comment

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

Should "should" be "must" ?

src/index.d.ts Outdated

/**
* A `(condition name, value)` map. The `condition_name` of the highest priority
* (the one listed first in the `RemoteConfig`'s conditions list) determines the value of
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be "RemoteConfig object's" ?

Or maybe "template's" ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. Changed the wording to The condition name of the highest priority (the one listed first in the Remote Config template's conditions list) determines the value of this parameter.

src/index.d.ts Outdated
/**
* The logic of this condition.
* See the documentation on
* {@link https://firebase.google.com/docs/remote-config/condition-reference Condition Expressions}
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be lowercase.

src/index.d.ts Outdated

/**
* The color associated with this condition for display purposes in the Firebase Console.
* Not specifying this value results in the Console picking an arbitrary color to associate
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be lowercase.

src/index.d.ts Outdated
/**
* Publishes a Remote Config template.
*
* @param template The Remote Config template to be validated.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be "published" ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch! Thanks!

Copy link
Contributor

@egilmorez egilmorez left a comment

Choose a reason for hiding this comment

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

Thanks!

@lahirumaramba lahirumaramba merged commit fce732c into remote-config Mar 26, 2020
@lahirumaramba lahirumaramba deleted the lm-remote-config-index branch March 26, 2020 15:10
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