From d8769ae1488f49030f1fd41681678dc6d5ca0483 Mon Sep 17 00:00:00 2001 From: James Daniels Date: Thu, 3 Dec 2020 12:51:05 -0500 Subject: [PATCH 1/4] fix(storage): fromTask on a running task should work as expected --- src/storage/observable/fromTask.ts | 34 ++++++++++++++++++++---------- 1 file changed, 23 insertions(+), 11 deletions(-) diff --git a/src/storage/observable/fromTask.ts b/src/storage/observable/fromTask.ts index ddbb9f388..360459f78 100644 --- a/src/storage/observable/fromTask.ts +++ b/src/storage/observable/fromTask.ts @@ -1,5 +1,4 @@ import { Observable } from 'rxjs'; -import { shareReplay } from 'rxjs/operators'; import { UploadTask, UploadTaskSnapshot } from '../interfaces'; // need to import, else the types become import('firebase/app').default.storage.UploadTask @@ -11,14 +10,27 @@ export function fromTask(task: UploadTask) { const progress = (snap: UploadTaskSnapshot) => subscriber.next(snap); const error = e => subscriber.error(e); const complete = () => subscriber.complete(); - task.on('state_changed', progress, (e) => { - progress(task.snapshot); - error(e); - }, () => { - progress(task.snapshot); - complete(); - }); - }).pipe( - shareReplay({ bufferSize: 1, refCount: false }) - ); + progress(task.snapshot); + switch (task.snapshot.state) { + case firebase.storage.TaskState.SUCCESS: + case firebase.storage.TaskState.CANCELED: + complete(); + break; + case firebase.storage.TaskState.ERROR: + error(new Error('task was already in error state')); + break; + default: + // on's type if Function, rather than () => void, need to wrap + const unsub = task.on('state_changed', progress, (e) => { + progress(task.snapshot); + error(e); + }, () => { + progress(task.snapshot); + complete(); + }); + return function unsubscribe() { + unsub(); + }; + } + }); } From f927abcd1913b1cf593f09da1f293add9fabb0fa Mon Sep 17 00:00:00 2001 From: James Daniels Date: Thu, 3 Dec 2020 14:23:50 -0500 Subject: [PATCH 2/4] Working better --- src/storage/observable/fromTask.ts | 31 +++++++++--------------------- src/storage/storage.spec.ts | 26 ++++++++++++++++++++++++- 2 files changed, 34 insertions(+), 23 deletions(-) diff --git a/src/storage/observable/fromTask.ts b/src/storage/observable/fromTask.ts index 360459f78..3cb0dafe3 100644 --- a/src/storage/observable/fromTask.ts +++ b/src/storage/observable/fromTask.ts @@ -10,27 +10,14 @@ export function fromTask(task: UploadTask) { const progress = (snap: UploadTaskSnapshot) => subscriber.next(snap); const error = e => subscriber.error(e); const complete = () => subscriber.complete(); - progress(task.snapshot); - switch (task.snapshot.state) { - case firebase.storage.TaskState.SUCCESS: - case firebase.storage.TaskState.CANCELED: - complete(); - break; - case firebase.storage.TaskState.ERROR: - error(new Error('task was already in error state')); - break; - default: - // on's type if Function, rather than () => void, need to wrap - const unsub = task.on('state_changed', progress, (e) => { - progress(task.snapshot); - error(e); - }, () => { - progress(task.snapshot); - complete(); - }); - return function unsubscribe() { - unsub(); - }; - } + const unsub = task.on('state_changed', progress); + task.then(snapshot => { + progress(snapshot); + complete(); + }, error); + // on's type if Function, rather than () => void, need to wrap + return function unsubscribe() { + unsub(); + }; }); } diff --git a/src/storage/storage.spec.ts b/src/storage/storage.spec.ts index 045077cba..877fcdd62 100644 --- a/src/storage/storage.spec.ts +++ b/src/storage/storage.spec.ts @@ -2,11 +2,12 @@ import { forkJoin } from 'rxjs'; import { mergeMap, tap } from 'rxjs/operators'; import { TestBed } from '@angular/core/testing'; import { AngularFireModule, FIREBASE_APP_NAME, FIREBASE_OPTIONS, FirebaseApp } from '@angular/fire'; -import { AngularFireStorage, AngularFireStorageModule, AngularFireUploadTask, BUCKET } from '@angular/fire/storage'; +import { AngularFireStorage, AngularFireStorageModule, AngularFireUploadTask, BUCKET, fromTask } from '@angular/fire/storage'; import { COMMON_CONFIG } from '../test-config'; import { rando } from '../firestore/utils.spec'; import { ChangeDetectorRef } from '@angular/core'; import 'firebase/storage'; +import firebase from 'firebase/app'; if (typeof XMLHttpRequest === 'undefined') { globalThis.XMLHttpRequest = require('xhr2'); @@ -64,13 +65,16 @@ describe('AngularFireStorage', () => { const blob = blobOrBuffer(JSON.stringify(data), { type: 'application/json' }); const ref = afStorage.ref(rando()); const task = ref.put(blob); + let lastSnap: firebase.storage.UploadTaskSnapshot; task.snapshotChanges() .subscribe( snap => { + lastSnap = snap; expect(snap).toBeDefined(); }, done.fail, () => { + expect(lastSnap.state).toBe('success'); ref.delete().subscribe(done, done.fail); }); }); @@ -104,6 +108,26 @@ describe('AngularFireStorage', () => { }).catch(done.fail); }); + it('should work with an already finished task', (done) => { + const data = { angular: 'promise' }; + const blob = blobOrBuffer(JSON.stringify(data), { type: 'application/json' }); + const ref = afStorage.storage.ref(rando()); + const task = ref.put(blob); + let lastSnap: firebase.storage.UploadTaskSnapshot; + task.then(_snap => { + fromTask(task).subscribe( + snap => { + lastSnap = snap; + expect(snap).toBeDefined(); + }, + done.fail, + () => { + expect(lastSnap.state).toBe('success'); + ref.delete().then(done, done.fail); + }); + }); + }); + }); describe('reference', () => { From 8eea9eb796834b0d5302a73e38ee6e1be517b678 Mon Sep 17 00:00:00 2001 From: James Daniels Date: Thu, 3 Dec 2020 15:31:21 -0500 Subject: [PATCH 3/4] Emit on first subscribe and debounceTime(0) --- src/storage/observable/fromTask.ts | 13 ++++++++++++- src/storage/storage.spec.ts | 6 ++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/src/storage/observable/fromTask.ts b/src/storage/observable/fromTask.ts index 3cb0dafe3..bdfb510ad 100644 --- a/src/storage/observable/fromTask.ts +++ b/src/storage/observable/fromTask.ts @@ -1,4 +1,5 @@ import { Observable } from 'rxjs'; +import { debounceTime } from 'rxjs/operators'; import { UploadTask, UploadTaskSnapshot } from '../interfaces'; // need to import, else the types become import('firebase/app').default.storage.UploadTask @@ -10,7 +11,13 @@ export function fromTask(task: UploadTask) { const progress = (snap: UploadTaskSnapshot) => subscriber.next(snap); const error = e => subscriber.error(e); const complete = () => subscriber.complete(); + // emit the current snapshot, so they don't have to wait for state_changes + // to fire next + progress(task.snapshot); const unsub = task.on('state_changed', progress); + // it turns out that neither task snapshot nor 'state_changed' fire the last + // snapshot before completion, the one with status 'success" and 100% progress + // so let's use the promise form of the task for that task.then(snapshot => { progress(snapshot); complete(); @@ -19,5 +26,9 @@ export function fromTask(task: UploadTask) { return function unsubscribe() { unsub(); }; - }); + }).pipe( + // deal with sync emissions from first emitting `task.snapshot`, this makes sure + // that if the task is already finished we don't emit the old running state + debounceTime(0) + ); } diff --git a/src/storage/storage.spec.ts b/src/storage/storage.spec.ts index 877fcdd62..754a606b5 100644 --- a/src/storage/storage.spec.ts +++ b/src/storage/storage.spec.ts @@ -65,16 +65,19 @@ describe('AngularFireStorage', () => { const blob = blobOrBuffer(JSON.stringify(data), { type: 'application/json' }); const ref = afStorage.ref(rando()); const task = ref.put(blob); + let emissionCount = 0; let lastSnap: firebase.storage.UploadTaskSnapshot; task.snapshotChanges() .subscribe( snap => { lastSnap = snap; + emissionCount++; expect(snap).toBeDefined(); }, done.fail, () => { expect(lastSnap.state).toBe('success'); + expect(emissionCount).toBeGreaterThan(0); ref.delete().subscribe(done, done.fail); }); }); @@ -113,16 +116,19 @@ describe('AngularFireStorage', () => { const blob = blobOrBuffer(JSON.stringify(data), { type: 'application/json' }); const ref = afStorage.storage.ref(rando()); const task = ref.put(blob); + let emissionCount = 0; let lastSnap: firebase.storage.UploadTaskSnapshot; task.then(_snap => { fromTask(task).subscribe( snap => { lastSnap = snap; + emissionCount++; expect(snap).toBeDefined(); }, done.fail, () => { expect(lastSnap.state).toBe('success'); + expect(emissionCount).toBe(1); ref.delete().then(done, done.fail); }); }); From 4c3a0fe6ae40416ec54cd69a01e07bcf3c79dc71 Mon Sep 17 00:00:00 2001 From: James Daniels Date: Thu, 3 Dec 2020 16:55:37 -0500 Subject: [PATCH 4/4] Tests and more comments --- src/storage/observable/fromTask.ts | 10 ++++++-- src/storage/storage.spec.ts | 39 ++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 2 deletions(-) diff --git a/src/storage/observable/fromTask.ts b/src/storage/observable/fromTask.ts index bdfb510ad..ab23aeada 100644 --- a/src/storage/observable/fromTask.ts +++ b/src/storage/observable/fromTask.ts @@ -6,13 +6,15 @@ import { UploadTask, UploadTaskSnapshot } from '../interfaces'; // and it no longer works w/Firebase v7 import firebase from 'firebase/app'; +// Things aren't working great, I'm having to put in a lot of work-arounds for what +// appear to be Firebase JS SDK bugs https://github.com/firebase/firebase-js-sdk/issues/4158 export function fromTask(task: UploadTask) { return new Observable(subscriber => { const progress = (snap: UploadTaskSnapshot) => subscriber.next(snap); const error = e => subscriber.error(e); const complete = () => subscriber.complete(); // emit the current snapshot, so they don't have to wait for state_changes - // to fire next + // to fire next... this is stale if the task is no longer running :( progress(task.snapshot); const unsub = task.on('state_changed', progress); // it turns out that neither task snapshot nor 'state_changed' fire the last @@ -21,7 +23,11 @@ export function fromTask(task: UploadTask) { task.then(snapshot => { progress(snapshot); complete(); - }, error); + }, e => { + // TODO investigate, again this is stale, we never fire a canceled or error it seems + progress(task.snapshot); + error(e); + }); // on's type if Function, rather than () => void, need to wrap return function unsubscribe() { unsub(); diff --git a/src/storage/storage.spec.ts b/src/storage/storage.spec.ts index 754a606b5..007a849ff 100644 --- a/src/storage/storage.spec.ts +++ b/src/storage/storage.spec.ts @@ -111,6 +111,45 @@ describe('AngularFireStorage', () => { }).catch(done.fail); }); + it('should cancel the task', (done) => { + const data = { angular: 'promise' }; + const blob = blobOrBuffer(JSON.stringify(data), { type: 'application/json' }); + const ref = afStorage.ref(rando()); + const task: AngularFireUploadTask = ref.put(blob); + let emissionCount = 0; + let lastSnap: firebase.storage.UploadTaskSnapshot; + task.snapshotChanges().subscribe(snap => { + emissionCount++; + lastSnap = snap; + if (emissionCount === 1) { + task.cancel(); + } + }, () => { + // TODO investigate, this doesn't appear to work... + // https://github.com/firebase/firebase-js-sdk/issues/4158 + // expect(lastSnap.state).toEqual('canceled'); + done(); + }, done.fail); + }); + + it('should be able to pause/resume the task', (done) => { + const data = { angular: 'promise' }; + const blob = blobOrBuffer(JSON.stringify(data), { type: 'application/json' }); + const ref = afStorage.ref(rando()); + const task: AngularFireUploadTask = ref.put(blob); + let paused = false; + task.pause(); + task.snapshotChanges().subscribe(snap => { + if (snap.state === 'paused') { + paused = true; + task.resume(); + } + }, done.fail, () => { + expect(paused).toBeTruthy(); + done(); + }); + }); + it('should work with an already finished task', (done) => { const data = { angular: 'promise' }; const blob = blobOrBuffer(JSON.stringify(data), { type: 'application/json' });