Update doc comments #5233
Update doc comments #5233
Conversation
hsubox76
requested review from
avolkovi,
Feiyang1,
hiranya911,
jsdt,
sam-gc,
schmidt-sebastian and
yuchenshi
as code owners
|
Changeset File Check ✅
|
Binary Size ReportAffected SDKs
Test Logs |
Size Analysis ReportAffected Products
|
There was a problem hiding this comment.
I got a start here, but it's so long that I think we should distribute the review among the writers focused in each area.
Please add Kevin, Rachel, Mark and Greg, and we can send them mail/chat instructions to take a close look at the files for their areas. SG? Thanks!
| @@ -59,11 +59,11 @@ declare module '@firebase/component' { | |||
| } | |||
|
|
|||
| /** | |||
| * Returns a Firebase Analytics instance for the given app. | |||
| * Returns a {@link Analytics} instance for the given app. | |||
There was a problem hiding this comment.
"Returns an" with "an," if the link text is just "Analytics," here and below.
| @@ -193,7 +193,7 @@ export function onTokenChanged( | |||
| onCompletion?: () => void | |||
| ): Unsubscribe; | |||
| /** | |||
| * Wraps addTokenListener/removeTokenListener methods in an Observer | |||
| * Wraps addTokenListener/removeTokenListener methods in an `Observer` | |||
There was a problem hiding this comment.
These should probably also be backticked.
| @@ -216,7 +216,7 @@ export function setAnalyticsCollectionEnabled( | |||
| ).catch(e => logger.error(e)); | |||
| } | |||
| /** | |||
| * Sends analytics event with given `eventParams`. This method | |||
| * Sends Google Analytics event with given `eventParams`. This method | |||
There was a problem hiding this comment.
Suggest "Sends a Google Analytics..."
Here and below.
| * See {@link https://developers.google.com/analytics/devguides/collection/ga4/display-features | Disable advertising features } | ||
| */ | ||
| 'allow_ad_personalization_signals'?: boolean; | ||
| [key: string]: unknown; | ||
| } | ||
|
|
||
| /** | ||
| * Analytics initialization options. | ||
| * Firebase Analytics instance initialization options. |
There was a problem hiding this comment.
Product-wise, we don't say "Firebase Analytics" any more -- just "Google Analytics."
There was a problem hiding this comment.
It's referring to the service, not the product, so I changed it to {@link Analytics}.
| @@ -358,7 +358,7 @@ export interface Persistence { | |||
| * Interface representing ID token result obtained from {@link User.getIdTokenResult}. | |||
| * | |||
| * @remarks | |||
| * It contains the ID token JWT string and other helper properties for getting different data | |||
| * It contains the ID token JWT string and other helper properties for getting different data | |||
There was a problem hiding this comment.
Should this be "The result" or something more clear than "It?"
There was a problem hiding this comment.
It's specifically this interface so I just put the name, in backticks?
egilmorez
requested review from
rachelsaunders,
markarndt,
kevinthecheung and
gtknoke
| @@ -88,7 +88,7 @@ export class TransactionResult { | |||
| * transaction will be aborted and the data at this location will not be | |||
| * modified. | |||
| * @param options - An options object to configure transactions. | |||
| * @returns A Promise that can optionally be used instead of the onComplete | |||
| * @returns A promise that can optionally be used instead of the `onComplete` | |||
There was a problem hiding this comment.
Either that, or just lowercase "promise." With a cap it's a literal (correct?) and with lowercase it's just a plain old promise, which I think devs will understand.
There was a problem hiding this comment.
- Uncapitalize "Promise" in several places since promises are standard parts of the language now? We don't capitalize
functionorclass.
This was my rationale for uncapitalizing it. I think comments were written in the past when promises weren't standard and so we had written an actual custom implementation of them at some point?
There was a problem hiding this comment.
Interesting! But lowercase seems to work well :)
There was a problem hiding this comment.
Promise is the name of the API and we write it in uppercase in code while we write function/class in lowercase. MDN uses capitalized Promise in their [doc]. so I think we should use literal Promise.
There was a problem hiding this comment.
A little hard to be sure I found all of them but I did as many as I could find.
| @@ -19,7 +19,8 @@ import { FirebaseApp, getApp, _getProvider } from '@firebase/app-exp'; | |||
| import { Installations } from '../interfaces/public-types'; | |||
|
|
|||
| /** | |||
| * Returns an instance of FirebaseInstallations associated with the given FirebaseApp instance. | |||
| * Returns an instance of Firebase Installations associated with the given | |||
There was a problem hiding this comment.
In long discussions when Andreas was still around, we decided not to capitalize "installations," even in the product name, unless heading case or something called for it -- so all of these instances should be lowercase.
These are the names/formats we agreed on: https://source.corp.google.com/piper///depot/google3/third_party/devsite/firebase/en/_internal/includes/_names.html;ws=codereview%2F392183348;rcl=378662242;cl=2;l=241
There was a problem hiding this comment.
It's referring to an instance of the SDK service so I went with {@link Installations}.
| @@ -51,13 +50,12 @@ export interface HttpsCallableOptions { | |||
| } | |||
|
|
|||
| /** | |||
| * `Functions` represents a Functions instance, and is a required argument for | |||
| * all Functions operations. | |||
| * An instance of Firebase Functions. | |||
There was a problem hiding this comment.
"Firebase Functions" isn't an actual product name.
Is there a reason not to just go with the literal here, like "A Functions instance?"
There was a problem hiding this comment.
Sounds good, done.
| @@ -41,7 +41,7 @@ export * from './src'; | |||
| * | |||
| * @public | |||
| */ | |||
| export const reactNativeLocalPersistence = | |||
| export const reactNativeLocalPersistence: Persistence = | |||
There was a problem hiding this comment.
Need to explicitly import the type here, or else rollup? typescript? transitively gets it from the file getReactNativePersistence comes from, and for some reason codes it as an import() statement in the d.ts, which makes api-extractor error.
|
Rebased and made some changes to get rid of warnings when running api-extractor in Auth. Able to get rid of all warnings except the |
functionorclass.Had to make some judgement calls where basically if it's being used as a product name, no backticks, if it's referring to the instance/class then backticks.
It is a little tricky to identify which comments in Firestore and RTDB go into the docs since they don't embed
@publictags and use another method to extract public APIs, so might have missed some.