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

Release Firestore Bundles as a prototype patched feature. #4168

Merged
merged 13 commits into from
Dec 8, 2020

Conversation

wu-hui
Copy link
Contributor

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

No description provided.

… as a separate module (#4120)

* Add bundles to d.ts and rearrange bundles source code for building it as a separate module.
Build firestore sdks with prebuilt to support bundle as a prototype patched feature.
@changeset-bot
Copy link

changeset-bot bot commented Dec 4, 2020

🦋 Changeset detected

Latest commit: e830a23

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 9 packages
Name Type
firebase Minor
@firebase/firestore-types Minor
@firebase/firestore Minor
@firebase/rules-unit-testing Patch

Not sure what this means? Click here to learn what changesets are.

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

wu-hui and others added 2 commits December 4, 2020 13:13
Add JSDoc for bundles.
# Conflicts:
#	packages/firestore/exp-types/index.d.ts
#	packages/firestore/package.json
@google-oss-bot
Copy link
Contributor

google-oss-bot commented Dec 4, 2020

Size Analysis Report

Affected Products

No changes between base commit (3754371) and head commit (4d9e963).

Test Logs

@github-actions
Copy link
Contributor

github-actions bot commented Dec 4, 2020

Changeset File Check ✅

  • No modified packages are missing from the changeset file.
  • No changeset formatting errors detected.

Copy link
Contributor

@schmidt-sebastian schmidt-sebastian left a comment

Choose a reason for hiding this comment

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

Basically LGTM, but I think the memory/bundle build might not work as is. Can you take a look?

{
input: {
index: path.resolve('./memory', memoryPkg['main-esm2017']),
bundle: path.resolve('./bundle', memoryBundlePkg['main-esm2017'])
Copy link
Contributor

Choose a reason for hiding this comment

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

If you still have the test app, can you verify the memory build works?

It should be "@firebase/firestore/memory" and @firebase/firestore/memory/bundle" (I believe).

I think we are still missing "memory/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.

@@ -138,3 +119,30 @@ export class LoadBundleTask
}
}
}

export function loadBundle(
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like we still need to add this to firestore-exp, but we can do this as a follow up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what this means, can you elaborate?

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like we are at least missing the top-level exports in exp/index.ts.

@@ -1394,10 +1395,11 @@ export function hasNewerBundle(

/**
* Saves the given `BundleMetadata` to local persistence.
* @param bundleMetadata
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing description. You can also just remove this line.

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.

@@ -1537,8 +1537,8 @@ export function ensureWriteCallbacks(syncEngine: SyncEngine): SyncEngineImpl {
* Loads a Firestore bundle into the SDK. The returned promise resolves when
* the bundle finished loading.
*
* @param bundleReader - Bundle to load into the SDK.
* @param task - LoadBundleTask used to update the loading progress to public API.
* @param bundleReader Bundle to load into the SDK.
Copy link
Contributor

Choose a reason for hiding this comment

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

API Extractor requires the dash. I don't like it either, but that's 2020 for you :)

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.

"@firebase/firestore": minor
---

Release Firestore Bundles as a prototype patched feature.
Copy link
Contributor

Choose a reason for hiding this comment

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

You should describe in your Release Notes what this is ("bundles" is not a term that developers will understand - at least not yet) and how they can use it (mention the additional import). The Prototype Patching is an implementation detail that doesn't need to go into the release notes. We should focus on usage.

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.

wu-hui and others added 3 commits December 4, 2020 21:26
# Conflicts:
#	packages/firestore/rollup.shared.js
* 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.
.changeset/old-lobsters-pull.md Outdated Show resolved Hide resolved
.changeset/old-lobsters-pull.md Outdated Show resolved Hide resolved
@@ -138,3 +119,30 @@ export class LoadBundleTask
}
}
}

export function loadBundle(
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like we are at least missing the top-level exports in exp/index.ts.

wu-hui and others added 2 commits December 7, 2020 14:22
Co-authored-by: Sebastian Schmidt <mrschmidt@google.com>
Co-authored-by: Sebastian Schmidt <mrschmidt@google.com>
Copy link
Contributor

@hsubox76 hsubox76 left a comment

Choose a reason for hiding this comment

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

LG, one comment for docs, adding @egilmorez for doc approval.

*
* The API is compatible with `Promise<LoadBundleTaskProgress>`.
*/
export interface LoadBundleTask extends PromiseLike<LoadBundleTaskProgress> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to create entries in https://github.com/firebase/firebase-js-sdk/blob/master/scripts/docgen/content-sources/js/toc.yaml for 2 new interfaces, LoadBundleTask and LoadBundleTaskProgress

Can just insert this alphabetically in the Firestore section:

  - title: "LoadBundleTask"
    path: /docs/reference/js/firebase.firestore.LoadBundleTask
  - title: "LoadBundleTaskProgress"
    path: /docs/reference/js/firebase.firestore.LoadBundleTaskProgress

If this is available for Node, also here:
https://github.com/firebase/firebase-js-sdk/blob/master/scripts/docgen/content-sources/node/toc.yaml

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.

@wu-hui wu-hui merged commit b662f8c into master Dec 8, 2020
@google-oss-bot google-oss-bot mentioned this pull request Dec 8, 2020
@firebase firebase locked and limited conversation to collaborators Jan 7, 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.

None yet

4 participants