Set up Storage modularization #3499

hsubox76 · 2020-07-28T21:24:42Z

Separating entry points into exp and compat versions.

Main implementation is exp but exp entry point is in a subfolder. Current version has wrapper classes in a compat folder, but package entry point is at top level.

Unit test files have been split into a "compat" version and an "exp" version when necessary.

The goal of this PR specifically is to have a functional backwards-compatible Storage SDK through the wrapper files in the compat/ dir, without the exp package being ready for public use yet.

In order to get the exp package ready for publishing we need to make additional PRs to:

Optimize for tree-shaking
Implement a separate non-resumable uploadBytes() function (this one includes `uploadBytesResumable())
Fix annotations to generate proper api report and docs.

changeset-bot · 2020-07-28T21:24:44Z

🦋 Changeset detected

Latest commit: b9751e2

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

This PR includes changesets to release 6 packages

Name	Type
@firebase/storage	Patch
firebase	Patch
@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

packages/storage/exp/index.ts

packages/storage/exp-types/index.d.ts

packages/storage/exp/api.ts

packages/storage/exp/index.ts

packages/storage/exp/package.json

packages/storage/src/reference.ts

packages/storage-types/exp/index.d.ts

packages/storage/compat/reference.ts

schmidt-sebastian · 2020-07-31T19:57:25Z

packages/storage/src/reference.ts

+ * @return A reference to the parent of the
+ *     current object, or null if the current object is the root.
+ */
+export function getParent(ref: Reference): Reference | null {


We need to figure out a name for this.

Put parent back as a method on Reference

Should we remove this method then?

Removed this function and inlined the code into get parent()

schmidt-sebastian · 2020-07-31T19:58:49Z

packages/storage/src/reference.ts

+ * Deletes the object at this location.
+ * @return A promise that resolves if the deletion succeeds.
+ */
+export function deleteObject(ref: Reference): Promise<void> {


In Firestore (and I believe Auth is doing this as well), we are taking the interface type and then use a casting helper to cast to the internal type. This ensures that someone doesn't use the old Reference in new code.

Is ReferenceCompat duck type compatible with Reference?

packages/storage/src/service.ts

hsubox76 · 2020-08-05T23:19:07Z

Not for review, just pushing code as-is to get some advice on if I'm going about the Typescript refactoring wrong. (Trying to explicitly implement the public API in the compat classes.)

googlebot · 2020-08-28T16:00:35Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

packages/storage-types/index.d.ts

packages/storage/compat/index.ts

packages/storage/compat/list.ts

schmidt-sebastian · 2020-09-25T17:12:08Z

packages/storage/compat/list.ts

+export class ListResultCompat implements types.ListResult {
+  constructor(
+    private readonly delegate: ListResult,
+    private converter: (ref: Reference) => ReferenceCompat


Is there a reason we cannot "hardcode" this converter in this class? We already seem to have cyclic references just because we imported the type.

It seems like it doesn't complain about cyclic references if it's only a type, but it does if you bring in actual code. Because types don't impact execution? Not sure the exact rationale.

Since this type cannot be constructed without passing in a factory function for References, it seems that in practice, these types are always included in the bundle?

packages/storage/compat/reference.ts

packages/storage/src/reference.ts

packages/storage/src/service.ts

packages/storage/src/task.ts

packages/storage/test/unit/reference.exp.test.ts

Feiyang1

Only reviewed the compat code so far. Will review the rest of the PR tomorrow.

common/api-review/storage.api.md

packages/storage-types/index.d.ts

packages/storage/compat/index.ts

packages/storage/compat/reference.ts

Feiyang1 · 2020-10-08T04:04:13Z

packages/storage/compat/reference.ts

+   * Updates the metadata for this object.
+   * @param metadata The new metadata for the object.


Just want to mention that we are changing to use tsdoc for documenting in vNext, which is used by api-documenter to generate reference docs. It is similar to jsdoc, but has some differences, for exmaple,
you need to put a - after the parameter name and it's returns instead of return. See https://api-extractor.com/pages/tsdoc/doc_comment_syntax/ for the complete syntax.

You don't have to change the comments for the existing code.

packages/storage/compat/index.ts

Feiyang1 · 2020-10-08T04:23:39Z

packages/storage/compat/service.ts

+    private converter: (ref: Reference) => ReferenceCompat
+  ) {}
+
+  app = this.delegate.app;


IIUC, we are not using @firebase/app-exp app yet. this.delegate.app is currently the @firebase/app app which is passed to StorageService here.

Before we officially release the modular SDK, we need to change the compat factory to something like:

function compatFactory(container) { const app = container.getProvider('app-compat').getImmediate()!; const storage = container.getProvider('storage').getImmediate()!; return new StorageServiceCompat(app, storage); }

The compat app should be passed to StorageServiceCompat as a constructor parameter.

packages/storage/compat/service.ts

packages/storage/compat/task.ts

packages/storage/exp/index.ts

packages/storage/rollup.config.exp.js

packages/storage/src/service.ts

packages/storage/src/task.ts

packages/storage/src/reference.ts

packages/storage/test/unit/reference.exp.test.ts

packages/storage/tsconfig.json

packages/storage/compat/reference.ts

packages/storage/compat/service.ts

packages/storage/compat/tasksnapshot.ts

packages/storage/package.json

Feiyang1 · 2020-10-22T23:16:55Z

packages/storage/src/reference.ts

@@ -54,9 +51,9 @@ import {
 *     the project ID of the base firebase.App instance.
 */
 export class Reference {
-  protected location: Location;
+  location: Location;


I think we should make them private as they appears to be internal implementations that outside should not have direct access to. We provide getters storage, bucket and fullPath to access them instead.

We can remove the @internal annotation if they are private.

packages/storage/src/service.ts

packages/storage/compat/reference.ts

schmidt-sebastian · 2020-10-23T22:31:02Z

packages/storage/compat/list.ts

+export class ListResultCompat implements types.ListResult {
+  constructor(
+    private readonly _delegate: ListResult,
+    private _referenceConverter: (ref: Reference) => ReferenceCompat


It feels simpler to pass in the Storage instance and "hardcode" the conversion to ReferenceCompat in this class.

I did it to get rid of the circular dependency warnings. Should we just ignore them? They look so messy.

Decided to have more readable code and live with a couple of circular dependency warnings.

packages/storage/compat/service.ts

schmidt-sebastian · 2020-10-23T22:36:41Z

packages/storage/src/reference.ts

+ * @return A reference to the parent of the
+ *     current object, or null if the current object is the root.
+ */
+export function getParent(ref: Reference): Reference | null {


Should we remove this method then?

packages/storage/src/task.ts

Feiyang1

LGTM. The PR contains some unrelated changes currently, you may need to merge with master to get a clean diff.

I would recommend generating a size report for the modular APIs as soon as possible, and whenever you make new changes, so you can check it against your expectations and fix issues (e.g. unexpected dependencies).

You probably want to use the cli directly for the first time because the PR integration doesn't work very well for new code.

schmidt-sebastian · 2020-11-03T23:26:06Z

packages/storage/compat/task.ts

+    private readonly _ref: ReferenceCompat
+  ) {}
+
+  snapshot = new UploadTaskSnapshotCompat(


Consider making this a getter.

schmidt-sebastian · 2020-11-03T23:28:10Z

packages/storage/compat/task.ts

+      | StorageObserver<UploadTaskSnapshot>
+      | undefined
+      | ((a: UploadTaskSnapshot) => unknown) = undefined;
+    if (nextOrObserver != null) {


This seems like code that someone might "fix" in the future by changing "!=" to "!==". This will then break the client. I usually just do !!nextOrObserver.

Optional.

schmidt-sebastian · 2020-11-03T23:29:35Z

packages/storage/compat/task.ts

+      } else {
+        wrappedNextOrObserver = {
+          next:
+            nextOrObserver.next == null


Same comment as above.

schmidt-sebastian · 2020-11-03T23:31:41Z

packages/storage/src/implementation/error.ts

 export function invalidArgument(
  index: number,
  fnName: string,
  message: string
+): FirebaseStorageError;


Note that there is only a single callsiste for invalidArgument with fnName and message. We could simplify this code by removing the outlier.

Ack, missed removing that param type check.

schmidt-sebastian · 2020-11-03T23:32:14Z

packages/storage/src/implementation/location.ts

 import { DEFAULT_HOST } from './constants';

 /**
- * @struct
+ * @public


Should we drop this?

schmidt-sebastian · 2020-11-03T23:33:23Z

packages/storage/src/implementation/string.ts

@@ -28,7 +29,7 @@ export const StringFormat = {
 };

 /**
- * @struct
+ * @internal


Again, I think we could drop this since this is in an internal source folder.

* Rolls a node app building bundles. * Build bundle files for given list of project IDs. * Build the bundle json map and save it for integration tests. * Add emulator_settings.ts to gulp * Move bundle.test.ts to api/ * Bundles passes all tests and expose in classic API * Add CI project ID to bundles. * Adhoc string replacement and length re-calculation * Fix lint errors. * Delete old changes from make node app * Address comments * Update yarn.lock for release (#3998) Temp fix where database version is bumped before firebase-admin can update deps * Manually prepares the bundle strings. * Update API * Update config.ts * Use Chrome for karma debugging (#4007) * Cache emulator between runs (#3956) * Remote Config Modularization (#3975) * rc exp init * Add apis * register rc exp * implement funcitonal APIs * fix tests * build rc exp * add api-extractor to rc types * cast directly witout function * delete changelog for rc exp * add code owners to rc exp * update dep version * Remove AuthErrorCode from core export (#4013) * Remove AuthErrorCode from core export * Api * Update config.ts to remove bundles * adds a root changelog (#4009) * adds a root changelog * Update CHANGELOG.md Co-authored-by: Feiyang <[email protected]> * Update dependency typescript to v4.0.5 (#3846) Co-authored-by: Renovate Bot <[email protected]> * Update dependency karma-firefox-launcher to v2 (#3987) Co-authored-by: Renovate Bot <[email protected]> * Update dependency google-closure-library to v20200830 (#3765) * Update dependency google-closure-library to v20200830 * Replace goog.isArray with Array.isArray https://github.com/google/closure-library/releases/tag/v20200628 Co-authored-by: Renovate Bot <[email protected]> Co-authored-by: Alex Volkovitsky <[email protected]> * exclude remote config exp packages in changeset (#4014) * Set 1s timeout for onBackgroundMessage Hook (#3780) * await onBackgroundMessage hook * Create fluffy-panthers-hide.md * block in onPush to let onBackgroundMessage to execute * polish wording * Update changeset to be more specific * Update fluffy-panthers-hide.md * Clarify PR is about a bug fix * Update fluffy-panthers-hide.md * A whole bunch of things to bring auth (compat) to parity (#3970) * Handle anonymous auth re-login edge case * Formatting * Initial fixes * Fix redirect * clean up additional user info * Formatting * PR feedback * Fix some tests * Fix tests & write some new ones * Fix broken build * Formatting * Formatting * PR feedback * Formatting * PR feedback Co-authored-by: avolkovi <[email protected]> * Add withFunctionsTriggersDisabled method to rules-unit-testing (#3928) * Update integration tests to use free functions * Functions compat package (#3739) * Add free functions to exports * Fix to avoid false failures on changeset checker (#4012) * Add changeset for Firestore (#4030) * Update functions-compat dep version and fix changeset script error (#4032) * Update integration tests. Minified tests fail. * Bump node memory limit for all test CI (#4035) * Compat Layer for Firestore (#4003) * Rename all public API types to PublicX (#4039) * Update all non-major dependencies (#3953) Co-authored-by: Renovate Bot <[email protected]> * Version Packages (#4033) Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * Set up Storage modularization (#3499) Refactor storage for modularization. * Free functions removed from exp database.ts Co-authored-by: Christina Holland <[email protected]> Co-authored-by: Sebastian Schmidt <[email protected]> Co-authored-by: Sam Stern <[email protected]> Co-authored-by: Feiyang <[email protected]> Co-authored-by: Sam Horlbeck Olsen <[email protected]> Co-authored-by: Dimitri Mitropoulos <[email protected]> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Renovate Bot <[email protected]> Co-authored-by: Alex Volkovitsky <[email protected]> Co-authored-by: Kai Wu <[email protected]> Co-authored-by: Google Open Source Bot <[email protected]> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

hsubox76 requested a review from schmidt-sebastian as a code owner July 28, 2020 21:24

hsubox76 force-pushed the ch-storage-exp branch from c940e93 to 38ca3bb Compare July 28, 2020 21:28

schmidt-sebastian reviewed Jul 28, 2020

View reviewed changes

packages/storage/exp/index.ts Show resolved Hide resolved

Feiyang1 reviewed Jul 28, 2020

View reviewed changes

schmidt-sebastian reviewed Jul 31, 2020

View reviewed changes

hsubox76 requested a review from hiranya911 as a code owner July 31, 2020 22:49

hsubox76 changed the base branch from master to storage-exp August 4, 2020 21:16

hsubox76 force-pushed the ch-storage-exp branch from bbfc41e to 396e809 Compare August 4, 2020 21:18

hsubox76 force-pushed the ch-storage-exp branch from e810bad to 570975e Compare August 28, 2020 15:59

hsubox76 requested review from alikn, andirayo, avolkovi, bojeil-google, davideast, egilmorez, erikeldridge, jamesdaniels, jsdt, sam-gc, scottcrossen, yuchenshi, zijianjoy and zwu52 as code owners August 28, 2020 15:59

hsubox76 changed the base branch from storage-exp to master August 28, 2020 16:00

hsubox76 removed request for erikeldridge, jamesdaniels and avolkovi August 28, 2020 16:22

hsubox76 force-pushed the ch-storage-exp branch 2 times, most recently from 3be9953 to 22c62e9 Compare September 22, 2020 18:21

schmidt-sebastian reviewed Sep 25, 2020

View reviewed changes

schmidt-sebastian assigned hsubox76 Sep 25, 2020

Feiyang1 reviewed Oct 8, 2020

View reviewed changes

hsubox76 mentioned this pull request Oct 14, 2020

Fix storage-types #3945

Merged

hsubox76 changed the title ~~WIP: Set up Storage modularization~~ Set up Storage modularization Oct 20, 2020

hsubox76 force-pushed the ch-storage-exp branch from d206430 to cf14b39 Compare October 20, 2020 16:22

Feiyang1 reviewed Oct 22, 2020

View reviewed changes

schmidt-sebastian reviewed Oct 23, 2020

View reviewed changes

hsubox76 requested a review from ChaoqunCHEN as a code owner October 27, 2020 17:21

Feiyang1 approved these changes Oct 28, 2020

View reviewed changes

hsubox76 force-pushed the ch-storage-exp branch from a4964b4 to 765c410 Compare October 29, 2020 17:07

hsubox76 removed the request for review from ChaoqunCHEN November 2, 2020 21:34

hsubox76 assigned schmidt-sebastian and unassigned hsubox76 Nov 3, 2020

schmidt-sebastian approved these changes Nov 3, 2020

View reviewed changes

schmidt-sebastian assigned hsubox76 and unassigned schmidt-sebastian Nov 3, 2020

hsubox76 force-pushed the ch-storage-exp branch from c079ef7 to 27bfcc5 Compare November 4, 2020 22:22

hsubox76 added 3 commits November 5, 2020 13:10

Storage modularization refactor.

b1267f0

Delete stray test files from merge

23d0e28

yarn.lock update

b9751e2

hsubox76 force-pushed the ch-storage-exp branch from 20a05fe to b9751e2 Compare November 5, 2020 21:28

hsubox76 merged commit f9dc50e into master Nov 6, 2020

schmidt-sebastian deleted the ch-storage-exp branch November 9, 2020 22:38

google-oss-bot mentioned this pull request Nov 10, 2020

Version Packages #4054

Merged

firebase locked and limited conversation to collaborators Dec 6, 2020

		* Updates the metadata for this object.
		* @param metadata The new metadata for the object.

Set up Storage modularization #3499

Set up Storage modularization #3499

Uh oh!

Conversation

hsubox76 commented Jul 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot bot commented Jul 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hsubox76 commented Aug 5, 2020

Uh oh!

googlebot commented Aug 28, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Feiyang1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hsubox76 commented Jul 28, 2020 •

edited

Loading

changeset-bot bot commented Jul 28, 2020 •

edited

Loading