Add nonresumable uploadBytes to modular Storage #4084

hsubox76 · 2020-11-17T19:17:58Z

Adding uploadBytes to storage-exp. This is a new method that does a non-resumable upload which requires less code than the resumable upload. We expect people to use this by default and only use the resumable version when needed, so it gets the default-sounding name and the resumable upload is named uploadBytesResumable.

uploadBytes has to generate an UploadTaskSnapshot with less properties (no task, state, bytesTransferred...do we still need totalBytes? not sure) so I created a separate class, and again gave it the default sounding name, while the old one is named UploadTaskResumableSnapshot.

changeset-bot · 2020-11-17T19:18:01Z

⚠️ No Changeset found

Latest commit: 91c5e61

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 · 2020-11-17T19:39:25Z

Size Analysis Report

Affected Products

No changes between base commit (8518b6b) and head commit (0cc7407).

Test Logs

Base (8518b6b2): https://github.com/firebase/firebase-js-sdk/actions/runs/406514897
Head (0cc74075): https://github.com/firebase/firebase-js-sdk/actions/runs/406871668

packages/storage/src/implementation/metadata.ts

packages/storage/src/implementation/requests.ts

Feiyang1 · 2020-11-17T21:42:32Z

packages/storage/src/implementation/requests.ts

+): RequestInfo<Metadata> {
+  const urlPart = location.bucketOnlyServerUrl();
+  const headers: { [prop: string]: string } = {
+    'Content-Type': 'application/octet-stream'


Should it always be octet-stream? the original logic indicates it's a fallback when metadata.contentType and blob.type() are undefined. Perhaps it doesn't matter since we can treat everything as arbitrary binary data?

(metadata && metadata['contentType']) || (blob && blob.type()) || 'application/octet-stream'

Normally (in the old resumable setup) the content type in the header doesn't actually match the metadata.contentType field because the metadata has a specific content type as seen in the logic above, and the request header Content-Type is always multipart. Since we're doing away with multipart I guess it would be best to make the header match the metadata.

packages/storage/src/reference.ts

yarn.lock

packages/storage/src/reference.ts

packages/storage/test/integration/integration.exp.test.ts

hsubox76 · 2020-11-18T00:42:23Z

Whoops, I misunderstood what multipartUpload does (it sends metadata alongside the data and is not related to resumableness), rewriting.

schmidt-sebastian

Looks pretty good (but somewhat hard to review due to the import/formatting changes). I wonder if you have been able to compute the size difference?

packages/storage/exp/index.ts

schmidt-sebastian · 2020-11-18T02:41:24Z

packages/storage/src/reference.ts

+ * Uploads data to this object's location.
+ * The upload is not resumable.
+ * @public
+ * @param ref - Storage Reference where data should be uploaded.


"Storage reference" or "StorageReference" ?

Hmm, exported type name is just Reference. I guess I should rename it since it's publicly exposed.

packages/storage/src/reference.ts

packages/storage/src/tasksnapshot.ts

packages/storage/test/integration/integration.exp.test.ts

schmidt-sebastian · 2020-11-19T16:24:02Z

packages/storage/compat/reference.ts

+    const data = dataFromString(format, value);
+    const metadataClone = { ...metadata } as Metadata;
+    if (metadataClone['contentType'] == null && data.contentType != null) {
+      metadataClone['contentType'] = data.contentType!;


Is the bang needed here? Doesn't the != check catch both null and undefined?

Removed, seems like it was left over from back when it was using type.isDef instead of != null.

schmidt-sebastian · 2020-11-19T16:25:46Z

packages/storage/package.json

+    "test:browser:compat:unit": "karma start --single-run --compat --unit",
+    "test:browser:exp:unit": "karma start --single-run --exp --unit",
+    "test:browser:compat:integration": "karma start --single-run --compat --integration",
+    "test:browser:exp:integration": "karma start --single-run --exp --integration",
+    "test:browser:compat": "karma start --single-run --compat",
+    "test:browser:exp": "karma start --single-run --exp",


Should we add these new test targets to the CI rule?

These were for development testing convenience, CI tests everything. If it's too messy I guess I can remove them, should be able to pass these args to yarn test:browser using -- I think.

Please leave them. It makes everyone's life easier.

schmidt-sebastian · 2020-11-19T16:27:09Z

packages/storage/src/reference.ts

 * @param data - The data to upload.
- * @param metadata - Metadata for the newly uploaded string.
+ * @param metadata - Metadata for the newly uploaded data.
+ * @returns An UploadTaskNonResumableSnapshot


Suggested change

* @returns An UploadTaskNonResumableSnapshot

* @returns An UploadTaskSnapshot

schmidt-sebastian · 2020-11-19T16:28:05Z

packages/storage/src/reference.ts

 * @param value - The string to upload.
 * @param format - The format of the string to upload.
- * @param metadata - Metadata for the newly uploaded object.
+ * @param metadata - Metadata for the newly uploaded string.
 * @returns An UploadTask that lets you control and


Suggested change

* @returns An UploadTask that lets you control and

* @returns UploadTaskSnapshot

schmidt-sebastian · 2020-11-19T16:29:29Z

packages/storage/src/reference.ts

+}
+
+/**
+ * Base code for nonresumable upload, used by uploadString and uploadBytes.


Suggested change

* Base code for nonresumable upload, used by uploadString and uploadBytes.

* Shared code for nonresumable upload, used by uploadString and uploadBytes.

Optional.

packages/storage/test/integration/integration.exp.test.ts

schmidt-sebastian · 2020-11-19T16:51:51Z

packages/storage/src/reference.ts

+
+/**
+ * Uploads data to this object's location.
+ * The upload is resumable and exposes progress updates.


Suggested change

* The upload is resumable and exposes progress updates.

* The upload can be paused, resumed and exposes progress updates.

Suggesting this because the Web SDK doesn't actually allow you to resume an upload with an Upload URL (as far as I can tell). Instead, the user can only resume uploads with an UploadTask that they paused.

Android let's users actually resume an upload with an Upload URL: https://firebase.google.com/docs/reference/android/com/google/firebase/storage/StorageReference#putFile(android.net.Uri,%20com.google.firebase.storage.StorageMetadata,%20android.net.Uri)

schmidt-sebastian · 2020-11-19T16:53:31Z

packages/storage/src/task.ts

+import {
+  createResumableUpload,
+  getResumableUploadStatus,
+  resumableUploadChunkSize,


Suggested change

resumableUploadChunkSize,

RESUMABLE_UPLOAD_CHUNK_SIZE,

schmidt-sebastian · 2020-11-19T16:55:17Z

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

+    it('uploadString throws', async () => {
      expect(() => uploadString(root, 'raw', StringFormat.RAW)).to.throw(
        'storage/invalid-root-operation'
      );
    });
+    it('uploadBytes throws', async () => {
+      expect(() => uploadBytes(root, new Blob(['a']))).to.throw(
+        'storage/invalid-root-operation'
+      );
+    });


Suggested change

it('uploadString throws', async () => {

expect(() => uploadString(root, 'raw', StringFormat.RAW)).to.throw(

'storage/invalid-root-operation'

);

});

it('uploadBytes throws', async () => {

expect(() => uploadBytes(root, new Blob(['a']))).to.throw(

'storage/invalid-root-operation'

);

});

it('uploadString throws', () => {

expect(() => uploadString(root, 'raw', StringFormat.RAW)).to.throw(

'storage/invalid-root-operation'

);

});

it('uploadBytes throws', () => {

expect(() => uploadBytes(root, new Blob(['a']))).to.throw(

'storage/invalid-root-operation'

);

});

schmidt-sebastian · 2020-11-19T16:57:56Z

packages/storage/compat/reference.ts

@@ -114,8 +115,17 @@ export class ReferenceCompat implements types.Reference {
    metadata?: Metadata
  ): types.UploadTask {
    this._throwIfRoot('putString');
+    const data = dataFromString(format, value);


Just curious - could this method just call put with new FbsBlob(data.data, true)?

You mean call uploadBytes instead of having a shared _nonResumableUpload function? I guess the only drawback would be an extra _throwIfRoot call that won't do anything.

Feiyang1 · 2020-11-19T19:17:21Z

common/api-review/storage.api.md

+// Warning: (ae-forgotten-export) The symbol "ListOptions" needs to be exported by the entry point index.d.ts
+// Warning: (ae-forgotten-export) The symbol "ListResult" needs to be exported by the entry point index.d.ts


We should address these warnings.

Partially addressed. There are a lot of files to fix so I will do a full pass on all files in another PR.

Feiyang1 · 2020-11-19T20:05:53Z

packages/storage/src/implementation/requests.ts

@@ -279,10 +277,13 @@ export function metadataForUpload_(
  return metadataClone;
 }

+/**
+ * Prepare RequestInfo for resumable uploads as Content-Type: multipart.


Suggested change

* Prepare RequestInfo for resumable uploads as Content-Type: multipart.

* Prepare RequestInfo for uploads as Content-Type: multipart.

It is used for both resumable and non-resumable uploads.

Thanks, fixed.

packages/storage/src/service.ts

Feiyang1 · 2020-11-19T20:09:01Z

packages/storage/src/tasksnapshot.ts

+export interface UploadTaskSnapshot {
+  readonly metadata: Metadata;
+  readonly ref: StorageReference;
+}
+
+export interface UploadTaskResumableSnapshot {
+  readonly bytesTransferred: number;
+  readonly totalBytes: number;
+  readonly state: TaskState;
+  readonly metadata: Metadata;
+  readonly task: UploadTask;
+  readonly ref: StorageReference;


Feiyang1 · 2020-11-19T20:21:34Z

res

@@ -0,0 +1,998 @@
+{


Feiyang1 · 2020-11-19T20:26:48Z

scripts/ci-test/build_changed.ts

+async function buildForTests(
+  config: TestConfig,
+  buildAppExp = false,
+  buildAppCompat = false


Was the script broken because of the missing argument until now? but the workflow seems to work fine?

It used to be accessible anyway because it was a global variable (not window global, like top level in the file) so it worked but maybe in an unexpected way.

schmidt-sebastian

LGTM from my end (please wait for Fei's LGTM as well). Consider calling uploadBytes from uploadString.

schmidt-sebastian · 2020-11-23T22:27:28Z

packages/storage/package.json

+    "test:browser:compat:unit": "karma start --single-run --compat --unit",
+    "test:browser:exp:unit": "karma start --single-run --exp --unit",
+    "test:browser:compat:integration": "karma start --single-run --compat --integration",
+    "test:browser:exp:integration": "karma start --single-run --exp --integration",
+    "test:browser:compat": "karma start --single-run --compat",
+    "test:browser:exp": "karma start --single-run --exp",


Please leave them. It makes everyone's life easier.

schmidt-sebastian · 2020-11-23T22:29:04Z

packages/storage/src/reference.ts

+ * @param ref - StorageReference where data should be uploaded.
+ * @param data - The data to upload.
+ * @param metadata - Metadata for the newly uploaded data.
+ * @returns An UploadTaskSnapshot


Suggested change

* @returns An UploadTaskSnapshot

* @returns An UploadTask

schmidt-sebastian · 2020-11-23T22:30:10Z

packages/storage/src/reference.ts

-  ref: Reference,
-  metadata: Metadata
+  ref: StorageReference,
+  metadata: Record<string, unknown>


What about Partial<Metadata>? I think that makes the intent very clear.

hsubox76 requested review from Feiyang1, hiranya911 and schmidt-sebastian as code owners November 17, 2020 19:17

Feiyang1 reviewed Nov 17, 2020

View reviewed changes

hsubox76 force-pushed the ch-storage-nonresumable branch from d8f09c1 to c4c6e32 Compare November 18, 2020 02:23

schmidt-sebastian reviewed Nov 18, 2020

View reviewed changes

schmidt-sebastian assigned hsubox76 Nov 18, 2020

schmidt-sebastian reviewed Nov 19, 2020

View reviewed changes

Feiyang1 reviewed Nov 19, 2020

View reviewed changes

hsubox76 assigned Feiyang1 and schmidt-sebastian and unassigned hsubox76 Nov 23, 2020

schmidt-sebastian approved these changes Nov 23, 2020

View reviewed changes

hsubox76 force-pushed the ch-storage-nonresumable branch from 2615765 to b0612cc Compare December 1, 2020 22:04

Feiyang1 approved these changes Dec 7, 2020

View reviewed changes

hsubox76 added 12 commits December 7, 2020 13:56

In progress

8d0ddc9

Remove exp integration test

4d4b689

wip

97e67fe

Set up tests, add comments

51f9721

Address PR comments

73d37a2

Remove simpleUpload

8ec3703

Fix tests

0ade8e7

Fix lint

018e4bd

Move code to shared functions

eec9a71

Address PR comments

a23fde2

Fix tests

bbf6ae6

Fix up comments for api extractor

22c2d07

hsubox76 added 5 commits December 7, 2020 13:56

Run api-report

2a2ccc2

Clean up paths/imports

b5fee1a

Inject SDK_VERSION into request

26cb244

Address PR comments

f30f6cd

Address PR comments

91c5e61

hsubox76 force-pushed the ch-storage-nonresumable branch from b0612cc to 91c5e61 Compare December 7, 2020 21:58

hsubox76 merged commit 9634efd into master Dec 7, 2020

firebase locked and limited conversation to collaborators Jan 7, 2021

hsubox76 deleted the ch-storage-nonresumable branch April 12, 2021 19:46

	* @returns An UploadTaskNonResumableSnapshot
	* @returns An UploadTaskSnapshot

	* @returns An UploadTask that lets you control and
	* @returns UploadTaskSnapshot

	* Base code for nonresumable upload, used by uploadString and uploadBytes.
	* Shared code for nonresumable upload, used by uploadString and uploadBytes.

	* The upload is resumable and exposes progress updates.
	* The upload can be paused, resumed and exposes progress updates.

		// Warning: (ae-forgotten-export) The symbol "ListOptions" needs to be exported by the entry point index.d.ts
		// Warning: (ae-forgotten-export) The symbol "ListResult" needs to be exported by the entry point index.d.ts

	* Prepare RequestInfo for resumable uploads as Content-Type: multipart.
	* Prepare RequestInfo for uploads as Content-Type: multipart.

Add nonresumable uploadBytes to modular Storage #4084

Add nonresumable uploadBytes to modular Storage #4084

Uh oh!

Conversation

hsubox76 commented Nov 17, 2020

Uh oh!

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

Uh oh!

⚠️ No Changeset found

Uh oh!

google-oss-bot commented Nov 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Size Analysis Report

Affected Products

Test Logs

Uh oh!

Uh oh!

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!

Uh oh!

hsubox76 commented Nov 18, 2020

Uh oh!

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

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!

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

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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

changeset-bot bot commented Nov 17, 2020 •

edited

Loading

google-oss-bot commented Nov 17, 2020 •

edited

Loading

hsubox76 Nov 18, 2020 •

edited

Loading

hsubox76 Nov 19, 2020 •

edited

Loading

hsubox76 Nov 19, 2020 •

edited

Loading