Skip to content
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

Add JSDoc for bundles #4155

Merged
merged 9 commits into from
Dec 4, 2020
Merged

Add JSDoc for bundles #4155

merged 9 commits into from
Dec 4, 2020

Conversation

wu-hui
Copy link
Contributor

@wu-hui wu-hui commented Dec 3, 2020

No description provided.

@changeset-bot
Copy link

changeset-bot bot commented Dec 3, 2020

⚠️ No Changeset found

Latest commit: 29e76c9

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Dec 3, 2020

Size Analysis Report

Affected Products

No changes between base commit (cc8dd9c) and head commit (29f957a).

Test Logs

packages/firebase/index.d.ts Outdated Show resolved Hide resolved
packages/firebase/index.d.ts Outdated Show resolved Hide resolved
packages/firebase/index.d.ts Outdated Show resolved Hide resolved
packages/firebase/index.d.ts Outdated Show resolved Hide resolved
export interface LoadBundleTaskProgress {
/** How many documents have been loaded. */
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that you removed these in the other PR. You should leave the comments in both places as firestore-exp builds its d.ts file from source.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that was a bad merge i think. Will keep the comments.

@markarndt
Copy link
Collaborator

Few edits sprinkled in above....

wu-hui and others added 5 commits December 3, 2020 16:58
Co-authored-by: Sebastian Schmidt <mrschmidt@google.com>
Co-authored-by: Sebastian Schmidt <mrschmidt@google.com>
Co-authored-by: Sebastian Schmidt <mrschmidt@google.com>
Co-authored-by: Sebastian Schmidt <mrschmidt@google.com>
/**
* Registers functions to listen to bundle loading progresses.
* @param next Called there is a progress update from bundle loading, typically whenever
* a Firestore document is loaded it will generate a progress update.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix indent to match the rest of this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

/**
* Loads a Firestore bundle into the local cache.
*
* @param bundleData An object representing the bundle to be load, could be a `ArrayBuffer`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

"...representing the bundle to be loaded. Valid objects are ArrayBuffer, ReadableStream<Uint8Array> or string."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

* a `ReadableStream<Uint8Array>` or a `string`.
*
* @return A `LoadBundleTask` object, which notifies callers with progress update, completion
* or error event. It can be used as a `Promise<LoadBundleTaskProgress>`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

"...notifies callers with progress updates, and completion or error events."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

): LoadBundleTask;

/**
* Reads a Firestore query from local cache that is associated to a given name.
Copy link
Collaborator

Choose a reason for hiding this comment

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

"Reads a Firestore named query from local cache."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here it actually reads a Query object, I added back ticks to make it clear.

/**
* Reads a Firestore query from local cache that is associated to a given name.
*
* The named queries are from bundles, and saved as a result of `loadBundle`. `namedQuery`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Something like:
"Named queries are a feature of Firestore bundles. Named queries and related data are packaged into a binary bundle on the server side, the bundle is fetched by the client over the network, and the bundle is loaded to local cache using the loadBundle method. Once in local cache, use this method to extract a query from the bundle by name. Once extracted, the named query is simply a Firestore Query object."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done..more or less similar.

@@ -8303,31 +8319,71 @@ declare namespace firebase.firestore {
INTERNAL: { delete: () => Promise<void> };
}

/**
* Represents the task of loading a Firestore bundle. It provides progress of the bundle
Copy link
Collaborator

Choose a reason for hiding this comment

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

"...provides progress of bundle loading, as well as task completion and error events."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

export interface LoadBundleTask {
/**
* Registers functions to listen to bundle loading progresses.
Copy link
Collaborator

Choose a reason for hiding this comment

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

"...to listen to bundle loading progress events."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

export interface LoadBundleTask {
/**
* Registers functions to listen to bundle loading progresses.
* @param next Called there is a progress update from bundle loading, typically whenever
Copy link
Collaborator

Choose a reason for hiding this comment

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

"Called when there is a.... Typically, next calls occur each time a Firestore document is loaded from the bundle."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

* Registers functions to listen to bundle loading progresses.
* @param next Called there is a progress update from bundle loading, typically whenever
* a Firestore document is loaded it will generate a progress update.
* @param error Called when there is an error occurred from loading the bundle. The task
Copy link
Collaborator

Choose a reason for hiding this comment

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

"Called when an error occurs during bundle loading...."

Hmm, if there are important edge cases in "there should be no more updates after this" then let's add?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Not sure what you mean by second statement.

/**
* Implements a `Promise<LoadBundleTaskProgress>` interface.
*
* @param onFulfilled It is called with the compeltion `LoadBundleTaskProgress` when the
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove "It is..." in both params and start with "Called..."

Are there status codes for LoadBundleTaskProgress termination?

For unFulfilled: "Called on completion of LoadBundleTaskProgress when completion status is XXX."
For unRejected: "Called on completion of LoadBundleTaskProgress when completion status is XXX."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no status code.

/**
* Implements a `Promise<LoadBundleTaskProgress>` interface.
*
* @param onRejected It is called when there is an error occurred from loading the bundle.
Copy link
Collaborator

Choose a reason for hiding this comment

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

See above...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, but without mentioning status code.

@wu-hui wu-hui assigned markarndt and unassigned wu-hui Dec 4, 2020
* Loads a Firestore bundle into the local cache.
*
* @param bundleData
* An object representing the bundle to be load. Valid objects are `ArrayBuffer`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

"...bundle to be loaded. Valid..."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

* `ReadableStream<Uint8Array>` or `string`.
*
* @return
* A `LoadBundleTask` object, which notifies callers with progress update, and completion
Copy link
Collaborator

Choose a reason for hiding this comment

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

"...with progress updates, and..."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

/**
* Registers functions to listen to bundle loading progress events.
* @param next
* Called when there is a progress update from bundle loading, typically `next` calls occur
Copy link
Collaborator

Choose a reason for hiding this comment

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

...from bundle loading. Typically, next calls..."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

* Implements the `Promise<LoadBundleTaskProgress>.then` interface.
*
* @param onFulfilled
* Called on the completion with a `LoadBundleTaskProgress` update when the
Copy link
Collaborator

Choose a reason for hiding this comment

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

This one was a little unclear. Maybe this edit will help, but please clarify "the completion of" what? Completion of the progress update?

"Called on completion of a LoadBundleTaskProgress update when..."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

): LoadBundleTask;

/**
* Reads a Firestore `Query` from local cache that is associated to a given name.
Copy link
Collaborator

Choose a reason for hiding this comment

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

"Reads a Firestore Query from local cache, identified by name."

"Bundles can contain named queries. These named queries are packaged..."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Collaborator

@markarndt markarndt left a comment

Choose a reason for hiding this comment

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

Great.

@wu-hui wu-hui merged commit 705b30f into wuandy/ReleaseBundles Dec 4, 2020
wu-hui added a commit that referenced this pull request Dec 8, 2020
* Add bundles to d.ts and rearrange bundles source code for building it as a separate module (#4120)

* Add bundles to d.ts and rearrange bundles source code for building it as a separate module.

* Roll bundle with prebuilt (#4128)

Build firestore sdks with prebuilt to support bundle as a prototype patched feature.

* Add JSDoc for bundles (#4155)

Add JSDoc for bundles.

* Create changeset

* Update old-lobsters-pull.md

* A few fixes for Firestore bundle sdk builds (#4177)

* Actually creates firebase/firestore/bundle package.json

* Fix standard node/browser builds for bundles

* Make it release from remote branch.

* Add bundle to firebase complete build and fix missing proto for memory build.

* Make rollup.config.js more organized.

* Revert "Make it release from remote branch."

This reverts commit 2c91fd1.

* 2017 -> 2020

* Update comment.

* Address comments

* Update .changeset/old-lobsters-pull.md

Co-authored-by: Sebastian Schmidt <mrschmidt@google.com>

* Update .changeset/old-lobsters-pull.md

Co-authored-by: Sebastian Schmidt <mrschmidt@google.com>

* Add toc.

* Remove webpack change.

Co-authored-by: Sebastian Schmidt <mrschmidt@google.com>
@firebase firebase locked and limited conversation to collaborators Jan 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants