-
Notifications
You must be signed in to change notification settings - Fork 2.2k
(WIP) Add AngularFireStorage #1369
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
Conversation
src/storage/observable/fromTask.ts
Outdated
|
||
export function fromTask(task: storage.UploadTask) { | ||
return new Observable<storage.UploadTaskSnapshot | undefined>(subscriber => { | ||
task.on('state_changed', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI I don't know if it was fixed since but I found that state_changed
is not updated right before complete()
in the JS SDK, say if you had a percentage uploader it would never hit 100%.
In my prior WIP branch I got this working nicely via:
task.on('state_changed', subscriber.next);
task.then(snapshot => {
subscriber.next(snapshot);
subscriber.complete();
}).catch(e => subscriber.error);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my docs before, make sure these all fire as expected:
this.state = task.map(s => s.state);
this.uploading = state.map(s => s === firebase.storage.TaskState.RUNNING);
this.success = state.map(s => s === firebase.storage.TaskState.SUCCESS);
this.percentage = task.map(s => s.bytesTransferred / s.totalBytes * 100);
That WIP was here FYI master...jamesdaniels:storage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pinging @sphippen!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Running and percentage are good. I can say that this.success = state.map(s => s === firebase.storage.TaskState.SUCCESS);
does not fire. The bug may still persist. Let's get Spencer's opinion on how to handle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, is the problem just that you don't get an event inside the 'state_change' stream that indicates 100% progress? Based on my understanding of the code, that shouldn't be happening. I'll test it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested some code that looks like this:
var task = ref.put(getFiles()[0]); task.on('state_changed', (snap) => { console.log(snap.bytesTransferred + ' / ' + snap.totalBytes); }, () => { console.log('error'); }, () => { console.log('done'); });
and the snapshot callback was called with bytesTransferred == totalBytes before the "done" callback was called.
Is that consistent with what you are seeing? I don't understand the context here very well, so it'd help if you could make a MCVE of the thing you consider a bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sphippen Your code sample works, the problem we are having is that the progress callback never fires for the enum firebase.storage.TaskState.SUCCESS
.
var task = ref.put(getFiles()[0]);
task.on('state_changed', (snap) => {
if(snap.state === firebase.storage.TaskState.SUCCESS) {
console.log('SUCCESS!');
}
}, () => { console.log('error'); }, () => { console.log('done'); });
Let us know if this is correct behavior to expect.
Thank you for the hard work on this, it's incredible news. The API looks clean and easy to use, just like the other AF2 modules. I especially liked the One small thing I see in the usage example up there, these lines: // get notified when the download URL is available
this.uploadURL = task.downloadURL(); I think you meant |
Thanks @javebratt! Just got that fixed! |
src/storage/observable/fromTask.ts
Outdated
import { Observable } from 'rxjs/Observable'; | ||
|
||
export function fromTask(task: storage.UploadTask) { | ||
return new Observable<storage.UploadTaskSnapshot | undefined>(subscriber => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking we should at least share()
this, perhaps even consider share(1)
or BehaviorSubject
. I foresee lots of tickets opened if we don't.
Also I haven't run + dug in but is this task lazy? E.g, won't start uploading until subscribed to? Will it double upload if cold? That may violate the expectations of beginners. In which case we should .share()
and then subscribe()
in here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. I'll check that and add tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to share this, since you're closing over the task, the task is effectively a Subject, since it multicasts anyhow.
@davideast I'd rather How about a |
@jamesdaniels In this case |
Hey @davideast , Is the In the regular JS SDK, I can do something like const storageRef: AngularFireStorageReference = this.afStorage
.ref(`${this.userId}/${billId}/billPicture/`);
return storageRef.putString(imageURL, 'base64', { contentType: 'image/png' })
.then(pictureSnapshot => {
// I use this to store the downloadURL in my database
}); But when I try to do the same with the new AF2 API I get: What would be the proper way of getting the downloadURL to store in the database after the upload is completed? UPDATE: Here's how I achieved it, don't know if this is the right approach for AF2: First, in the provider I separated it into two functions: takeBillPhoto(billId: string, imageURL: string): AngularFireUploadTask {
const storageRef: AngularFireStorageReference = this.afStorage.ref(
`${this.userId}/${billId}/billPicture/`
);
return storageRef.putString(imageURL, 'base64', {
contentType: 'image/png'
});
}
storeDownloadUrl(billId: string, downloadUrl: string): Promise<any> {
console.log(billId, downloadUrl);
return this.billList.update(billId, { picture: downloadUrl });
} And from my component I'm calling them like this: this.billProvider
.takeBillPhoto(this.billId, imageData)
.then()
.then(res => {
this.billProvider.storeDownloadUrl(
this.billId,
res.metadata.downloadURLs[0]
);
}); It successfully uploads the picture to Firebase Storage, once the picture is uploaded it calls the other method and passes the download URL so I can store it inside my object in the database. But it feels like I'm making stuff up, don't know if I'm missing something |
Looks great overall. Threw together a demo at https://angular-z2ucyg.stackblitz.io and noticed a couple issues.
|
Will there be an offline feature like in Firestore or Firebase? Thanks. |
@davideast I'm not experience enough to review this. Just want to say thanks for the great work. It works flawlessly so far. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments.
src/storage/observable/fromTask.ts
Outdated
import { Observable } from 'rxjs/Observable'; | ||
|
||
export function fromTask(task: storage.UploadTask) { | ||
return new Observable<storage.UploadTaskSnapshot | undefined>(subscriber => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to share this, since you're closing over the task, the task is effectively a Subject, since it multicasts anyhow.
src/storage/task.ts
Outdated
} | ||
|
||
export function createUploadTask(task: storage.UploadTask): AngularFireUploadTask { | ||
const inner$ = fromTask(task).pipe(shareReplay()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that fromTask(task)
is going to return a multicast observable by default (since it doesn't create the task, rather, it wraps it), you probably don't need the shareReplay
unless you're trying to get a caching behavior. What is the use case where you will need that caching behavior? To be specific, you're talking about a scenario where the fromTask observable completes, but you want to subscribe to it later and get the value(s) out of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a file upload upload, I'd like to keep from uploading it again. If a user subscribes multiple times it be ideal to replay the state rather than upload the file again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might want to let your users compose that in. Otherwise, if they want to remove caching, they might have a hard time. (A take(1)
should do it) But you'll be taking away from them a primitive building block.
A use case might help me understand though.
src/storage/observable/fromTask.ts
Outdated
e => subscriber.error(e), | ||
() => subscriber.complete() | ||
); | ||
return { unsubscribe: task.cancel }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should just return a function here. Also you're going to want to retain the handler you added above with task.on('state_changed', handler)
so you can remove it (presumably with off
) otherwise you could end up with memory leaks.
Great work! Though I think there is an issue with the downloadURL Observerable, if the file is large (few mb), download URL is always null.
Anyone else having this issue? |
@slyedoc, I am having the same issue. I thought it was interesting that it was defined as being either string | null. Wonder when and why the null code path is executed. |
@slyedoc same here :-/ |
This is awesome, great work! Is there a way to retrieve the names of the files that reside in a specific folder in storage? |
@slyedoc @jkossis @anonymuos1 This is a bug in the core SDK. If the file is larger than 256k it is uploaded in chunks. This is handled differently and the downloadURL is not given in the progress callback, but in the completed callback. I'm going to make the change in the core SDK and it should work once that change lands. |
The wait is finally over! The
AngularFireStorage
service is here.Closes #940.
What's it all about?
Does you website need to handle user generated content like images or other media? Cloud Storage for Firebase is a serverless way of managing binary files. Now with an AngularFire integration, it's as easy as a few method calls.
Next release
Give it a spin by downloading the
@next
release from npm.Demo App
Check out the demo app, which provides you the ability to select a file, see the preview, upload, see the progress, and get the download URL.
Getting started
AngularFire breaks up Firebase features into separate modules. This reduces bundle size by including only the features your app needs.
Example usage
API Surface
Uploading files
There are two options for uploading files.
put()
orputString()
methods..upload()
method which implicitly creates a named reference for you and starts the upload process.Downloading files
To download a file you'll need to create a reference and call the
getDownloadURL()
method.Managing metadata
Cloud Storage for Firebase allows you to upload and download metadata associated with files. This is useful because you can store important metadata and download it without needing to download the entire file.
Downloading metadata
Uploading metadata with files
Firebase Source (WIP!)
We're going to add a
[firebaseSrc]
directive. This directive allows you to provide a Cloud Storage path and have it automatically resolve the download url to thesrc
attribute. This directive is a big WIP and may not make this release. However, your feedback on it is really important.Feedback requested!
We would love your feedback on the initial API. What do you like, dislike, and what have we missed?