New: Chunked uploads by tonyjin · Pull Request #15 · box/box-ui-elements

tonyjin · 2017-07-11T20:03:29Z

Files >50MB will be uploaded with the chunked uploads API by default
The 'chunked' Content Uploader option can be set to false to disable chunked uploads
Chunk upload failures due to rate limiting or network errors will be retried until success

priyajeet

Still 1 file left to review, the big one

priyajeet · 2017-07-11T20:44:41Z

+
+class Chunk extends Base {
+    cancelled: boolean;
+    chunk: Object | null;


I think you can do ?Object, then | null can be removed

priyajeet · 2017-07-11T20:46:45Z

+    data: Object = {};
+    progress: number = 0;
+    retry: number;
+    uploadHeaders: Object;


type StringMap from flowTypes

priyajeet · 2017-07-11T20:50:00Z

+     * @returns {void}
+     */
+    upload(): void {
+        if (this.cancelled) {


If subclassing Base.js, it has the concept of this.isDestroyed(), can reuse that. See other API files.

priyajeet · 2017-07-11T20:50:40Z

+        clearTimeout(this.retry);
+        this.chunk = null;
+        this.data = {};
+        this.cancelled = true;


this.destroy() can be used here

priyajeet · 2017-07-11T20:51:52Z

+
+const UPLOAD_RETRY_INTERVAL_MS = 1000;
+
+class Chunk extends Base {


Any reason this needs to be a child of Base?

Auth information + needs its own XHR.

priyajeet · 2017-07-11T20:53:27Z

+ * @return {string}
+ */
+/* eslint-disable no-bitwise */
+export default function(numArray: Int32Array): string {


Probably add a test for this file while you are at it, you can steal it from jsperf

priyajeet · 2017-07-11T20:56:11Z

                delete hdrs.Accept;
                delete hdrs['Content-Type'];

+                if (headers['Content-Type']) {


Content-Type string being repeated 4+ times

priyajeet · 2017-07-11T20:58:57Z

-        Object.keys(data).forEach((key) => {
-            formData.append(key, data[key]);
-        });
+    postFile({


Probably can rename to upload or something, since its PUT/POST both

priyajeet · 2017-07-11T21:02:48Z

-        this.destroy();
-        this.uploadAPI = new UploadAPI(this.options);
+    getUploadAPI(chunked?: boolean, fileSize?: number): ChunkedUploadAPI | PlainUploadAPI {
+        if (this.uploadAPI) {


Any reason why you are returning the prior one?
What if it was destroyed()?

I need to be able to cancel - fetching a different API won't cancel the right one.

priyajeet · 2017-07-11T21:14:09Z

+        this.chunks = [];
+
+        // Abort upload session
+        this.xhr.delete(`${this.uploadHost}/api/2.0/files/upload_sessions/${this.sessionId}`);


Maybe have this as a function somewhere, I think there is another place its used

priyajeet · 2017-07-11T21:15:05Z

+     * @returns {void}
+     */
+    cancel(): void {
+        // Cancel individual upload chunks


Probably add destroyedCheck()

priyajeet · 2017-07-11T21:15:38Z

+     * @return {Promise}
+     */
+    getNextChunk(): Promise<any> {
+        return new Promise((resolve, reject) => {


Should add destroyedCheck

priyajeet · 2017-07-11T21:18:51Z

+            } else {
+                // Otherwise, reupload and append timestamp
+                // 'test.jpg' becomes 'test-TIMESTAMP.jpg'
+                const extension = name.substr(name.lastIndexOf('.')) || '';


bug when file has no extension

you can also pass in the name from the file object
also see RenameDialog.js

Known issue - there's no good cross-browser way to distinguish between a file/folder from the File API so for now I'm using the no extension = folder heuristic

priyajeet · 2017-07-11T21:20:48Z

+        // Use provided upload URL if passed in, otherwise construct
+        let uploadSessionUrl = url;
+        if (!uploadSessionUrl) {
+            uploadSessionUrl = `${this.uploadHost}/api/2.0/files/upload_sessions`;


Creating a url can be moved to base.js along with the other api one

priyajeet · 2017-07-11T21:21:56Z

+        }
+
+        for (let i = 0; i < UPLOAD_PARALLELISM; i += 1) {
+            this.getNextChunk().then((chunk) => (chunk ? this.uploadChunk(chunk) : this.commitFile())).catch(() => {


Should catch just call this.uploadSessionErrorHandler?

No, conceptually different - but this is unrecoverable though so I'll call the error callback

priyajeet · 2017-07-11T21:22:53Z

+
+            // Read the blob as an ArrayBuffer so SHA1 can be calculated with Rusha
+            const fileReader = new FileReader();
+            fileReader.addEventListener('load', (event: any) => {


event: Event & { target: EventTarget }

What is this syntax exactly?

priyajeet · 2017-07-11T21:23:30Z

+                resolve(chunk);
+            });
+
+            fileReader.addEventListener('error', () => {


fileReader.addEventListener('error', reject) ?

priyajeet · 2017-07-11T21:24:28Z

+     *
+     * @return {Promise}
+     */
+    getNextChunk(): Promise<any> {


Promise<Chunk> works?

- Files >50MB will be uploaded with the chunked uploads API by default - The 'chunked' Content Uploader option can be set to false to disable chunked uploads - Chunk upload failures due to rate limiting or network errors will be retried until success

priyajeet reviewed Jul 11, 2017

View reviewed changes

New: Chunked uploads

28ec485

- Files >50MB will be uploaded with the chunked uploads API by default - The 'chunked' Content Uploader option can be set to false to disable chunked uploads - Chunk upload failures due to rate limiting or network errors will be retried until success

tonyjin merged commit 59feabc into box:master Jul 12, 2017

tonyjin deleted the supercharged-uploads branch July 12, 2017 00:40


		const UPLOAD_RETRY_INTERVAL_MS = 1000;

		class Chunk extends Base {

Conversation

tonyjin commented Jul 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

priyajeet left a comment

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!

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!

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!

priyajeet Jul 11, 2017 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tonyjin commented Jul 11, 2017 •

edited

Loading

priyajeet Jul 11, 2017 •

edited

Loading