feat(storage): Firebase Storage for AngularFire #865

davideast · 2016-10-18T17:04:11Z

Firebase Storage for AngularFire!

Addresses #785. This is a WIP PR, do not merge yet.

Description

This PR adds two new services to AngularFire, $firebaseStorage and the [firebase-src] directive.

API Reference

interface $firebaseStorage {
  (ref: firebase.storage.Reference): FirebaseStorage;
}

interface FirebaseStorage {
  constructor (storageRef: firebase.storage.Reference);
  $put(File, Metadata): UploadTask;
  $putString(string, format, Metadata): UploadTask;
  $getDownloadURL(): Promise<string>;
  $getMetadata(): Promise<Object>;
  $updateMetadata(): Promise<Object>;
  $delete(): Promise<void>;
  $toString(): string;
}

interface UploadTask {
  $progress: (Object) => (); 
  $error: (Error) => (); 
  $complete: (Object) => (); 
  $cancel(): boolean;
  $resume(): boolean;
  $pause(): boolean;
  $snapshot(): StorageSnapshot;
  then: Promise<Object>;
  catch: Promise<Error>;
}

Code sample

function MyCtrl($scope, $firebaseStorage) {
  const storageRef = firebase.storage().ref('thing');
  const $storage = $firebaseStorage(storageRef);
  const $task = $storage.$put(someFile);
  $task.$progress(snapshot => {

  });
  $task.$complete(snapshot => {
  
  });
}

<div class="image-holder">
   <!-- firebase-src automatically downloads from Firebase Storage -->
   <img firebase-src="thing" />
</div>

googlebot · 2016-10-18T17:04:12Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

googlebot · 2016-10-18T17:04:12Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

CyrusZei · 2016-10-18T17:13:02Z

omg man, I need to test this :D

soumak77 · 2016-11-08T21:36:15Z

I would like to propose creating a service similar to $firebaseRef but for storage references. I really like how $firebaseRef abstracts the exact paths to data and would like the same functionality for storage.

Currently the firebase-src directive takes a path to the data via the gs-url attribute. An alternate attribute could be added to allow passing a key for the storage reference.

soumak77 · 2016-11-08T21:47:01Z

src/storage/FirebaseStorageDirective.js

+        // $observe is like $watch but it waits for interpolation 
+        // Ex: <img firebase-src="{{ myUrl }}"/> 
+        attrs.$observe('firebaseSrc', function (newVal) {
+          if (newVal !== '' && newVal !== null && newVal !== undefined) {


I'm not sure I understand how this directive should work. It appears the directive's purpose is to generate the url to the file based on a path to the file. This path looks to be provided by the gs-url attribute. The code though is setup to watch the value myUrl passed in by the user. What exactly is the user suppose to be passing to the directive? It appears the directive should be using the observed value of the firebase-src attribute instead of the gs-url attribute.

This is an error from an earlier prototype. I have it fixed in a local version. I'm working on some final tests and hope to have it submitted soon!

@davideast I see you made the update, but changed it to use attrs.firebaseSrc instead of the interpolated value newVal. Does that work?

Disregard my last comment. I just tested with the recent changes and all appears to be working.

davideast · 2016-11-09T13:01:33Z

@soumak77 I too would like to have a provider for storage references! This release though will be dedicated to the core storage functionality. I think we could do that in the next release though 😄

…ests for storage

CyrusZei · 2016-11-09T15:18:26Z

Oh man @davideast i am following all your updates to this firebase Storage and cant wait until I can try it out. Keep up the good work man, love it

soumak77 · 2016-11-09T23:54:42Z

@davideast Could you add a firebase-href directive that works exactly the same as the firebase-src directive, except that it sets the href attribute with the file url instead of the src attribute?

The use case for such a directive would be when you want to download the file from firebase and only have a path:

<a download="" firebase-href="{{ path }}">download</a>

This mirrors the usage of firebase-src for viewing the file:

<img firebase-src="{{ path }}"></img>

CyrusZei · 2016-11-16T14:35:44Z

Is there by any chance that I can try this out ? and maybe add a example ? The storage feature is the only thing that I feel like I am missing. Firebase is so damn awsome.

jwngr · 2016-12-20T22:04:51Z

@davideast - FYI that if you merge your branch with master, the tests should properly run and pass on Travis thanks to #891.

googlebot · 2016-12-23T23:08:39Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

coveralls · 2017-01-06T14:49:32Z

Coverage increased (+3.7%) to 93.69% when pulling 8bd3a44 on storage into c22ef72 on master.

coveralls · 2017-01-09T12:56:37Z

Coverage increased (+3.7%) to 93.69% when pulling 4cdd779 on storage into c22ef72 on master.

jwngr · 2017-01-09T17:56:13Z

package.json

@@ -11,6 +11,9 @@
  "bugs": {
    "url": "https://github.com/firebase/angularfire/issues"
  },
+  "scripts": {
+    "start": "grunt"


Let's remove it since we got the Travis thing figured out and in Travis, we need to run a special script anyway, not just grunt.

jwngr · 2017-01-09T17:56:53Z

src/storage/FirebaseStorage.js

+        });
+      },
+      $error: function $error(callback) {
+        task.on('state_changed', function () {}, function (err) {


Suggestion: for clarity, use null instead of function () {}.

jwngr · 2017-01-09T17:57:38Z

src/storage/FirebaseStorage.js

+      $progress: function $progress(callback) {
+        task.on('state_changed', function () {
+          $digestFn(function () {
+            callback([unwrapStorageSnapshot(task.snapshot)]);


I don't think you want the brackets ([]) here. Should just be callback(unwrapStorageSnapshot(task.snapshot));. Same for the callbacks below.

jwngr · 2017-01-09T18:02:36Z

src/storage/FirebaseStorage.js

+
+  function _assertStorageRef(storageRef) {
+    if (!isStorageRef(storageRef)) {
+      throw new Error('$firebaseStorage expects a storage reference from firebase.storage().ref()');


Suggestion: "$firebaseStorage expects a Storage reference"

Capitalizing "Storage"

Taking out "from firebase.storage().ref()" since it could also come from "app.storage().ref()"

jwngr · 2017-01-09T18:29:42Z

src/storage/FirebaseStorage.js

+
+    Storage.utils = {
+      _unwrapStorageSnapshot: unwrapStorageSnapshot,
+      _$put: _$put,


Doest _$put() need to be exposed on Storage.utils? Shouldn't having Storage.$put() be enough?

jwngr · 2017-01-09T18:54:02Z

tests/protractor/upload/upload.manual.js

+    expect(browser.getTitle()).toEqual('AngularFire Upload e2e Test');
+  });
+
+  it('starts with an empty list of messages', function () {


This test seems unneeded for the upload test suite.

jwngr · 2017-01-09T18:54:45Z

tests/protractor/upload/upload.manual.js

+  });
+
+  it('uploads a file', function (done) {
+    var fileToUpload = './upload/logo.png',


Indentation for this whole test is off. Also, please use a new var / line for each variable.

jwngr · 2017-01-09T18:57:07Z

tests/unit/FirebaseStorage.spec.js

+
+      });
+
+      describe('_$put', function () {


As noted above, we shouldn't need to expose this method and should just move these tests under the public $put method.

jwngr · 2017-01-09T18:59:43Z

tests/unit/FirebaseStorage.spec.js

+          // test that $firebaseStorage.$delete() calls storageRef.delete()
+          var ref = firebase.storage().ref('thing');
+          var storage = $firebaseStorage(ref);
+          var fakePromise = $q(function(resolve, reject) {


Unused variable.

jwngr · 2017-01-09T19:00:01Z

tests/unit/FirebaseStorage.spec.js

+
+      describe('$delete', function() {
+        it('should call the storage ref delete method', function() {
+          // test that $firebaseStorage.$delete() calls storageRef.delete()


Unneeded comment.

coveralls · 2017-01-10T14:03:56Z

Coverage increased (+3.7%) to 93.673% when pulling da9a148 on storage into c22ef72 on master.

jwngr

Okay, I think this is the last major set of review comments. I took a close look at the API of the underlying Firebase SDK and think we should make a few changes to methods to better align there. Other than that, everything looks good!

jwngr · 2017-01-10T17:19:51Z

src/storage/FirebaseStorage.js

+(function() {
+  "use strict";
+
+  function unwrapStorageSnapshot(storageSnapshot) {


Style nit: You have _assertStorageRef() below, but don't use a leading underscore in front of unwrapStorageSnapshot() and isStorageRef(). Let's put a leading underscore in front of each of them for consistency.

jwngr · 2017-01-10T17:26:24Z

src/storage/FirebaseStorage.js

+    var Storage = function Storage(storageRef) {
+      _assertStorageRef(storageRef);
+      return {
+        $put: function $put(file) {


I gave a look through the API reference for firebase.storage.Reference, and I notice there are two methods which we do not have listed here:

putString() - I think this method was added after you started this proposal, but we should include it since it allows for some use cases that the regular put() doesn't cover. It should be easy enough to refactor the code you wrote for $put() to implement $putString() as well.

toString() - I assume we don't technically need to include this since it is a synchronous method and you could always just use the underlying method on the storageRef you pass in here, but it is probably good to include a $toString() method here for completeness.

jwngr · 2017-01-10T17:35:09Z

src/storage/FirebaseStorage.js

+            $pause: task.pause,
+            then: task.then,
+            catch: task.catch,
+            _task: task


It looks like in the underlying SDK, UploadTask has a snapshot property. Since we are returning an object which is the AngularFire version of UploadTask, we should probably have $snapshot: task.snapshot here instead of _task: task.

jwngr · 2017-01-10T17:36:55Z

src/storage/FirebaseStorageDirective.js

+        // $observe is like $watch but it waits for interpolation
+        // Ex: <img firebase-src="{{ myUrl }}"/>
+        attrs.$observe('firebaseSrc', function (newFirebaseSrcVal) {
+          if (newFirebaseSrcVal !== '' &&


Sorry, I think my comment last time around was confusing. This conditional can be simplified to:

if (typeof newFirebaseSrcVal === 'string' && newFirebaseSrcVal !== '')

Also, if the passed value of newFirebaseSrcVal is invalid, do we want to throw an error? Seems like we are just silently failing in that case which will be hard for developers to track down.

Actually, upon further testing and research I found out the attrs.$observe converts any value into a string. Therefore if you pass in an object, number, or bool it is automatically handled which makes the string check extra work. If you pass a null or undefined it is converted to an empty string, which is the only thing we need to check for.

jwngr · 2017-01-10T17:39:49Z

tests/protractor/upload/upload.js

+var app = angular.module('upload', ['firebase.storage']);
+
+app.controller('UploadCtrl', function Upload($scope, $firebaseStorage, $timeout) {
+  // Create a reference (possible create a provider)


Is the part of this comment within parentheses required?

jwngr · 2017-01-10T17:40:51Z

tests/protractor/upload/upload.js

+  $scope.upload = function() {
+    $scope.isUploading = true;
+    $scope.metadata = {bytesTransferred: 0, totalBytes: 1};
+    $scope.error = {};


Suggestion: use null instead of {}.

Also, we never actually use $scope.error in the HTML file. We should add a conditional <div> which shows the error if there is one.

coveralls · 2017-01-11T13:46:10Z

Coverage increased (+3.7%) to 93.716% when pulling e584905 on storage into c22ef72 on master.

jwngr

LGTM with four last minor testing related comments.

jwngr · 2017-01-11T18:34:02Z

tests/unit/FirebaseStorage.spec.js

+          task.catch();
+          expect(mockTask.catch).toHaveBeenCalled();
+        });
+


Please add a test for $snapshot as well.

jwngr · 2017-01-11T18:34:49Z

tests/unit/FirebaseStorage.spec.js

+      });
+
+      describe('$putString', function() {
+        it('should call a storage ref put', function () {


s/put/putString

jwngr · 2017-01-11T18:38:37Z

tests/unit/FirebaseStorage.spec.js

+      });
+    });
+
+    function setupPutTests(file, mockTask, isPutString) {


s/file/fileOrRawString

jwngr · 2017-01-11T18:41:23Z

tests/unit/FirebaseStorage.spec.js

+      } else {
+        spyOn(ref, putMethod);
+      }
+      task = storage['$' + putMethod](file);


In the case of isPutString, let's actually pass "raw" as the second parameter after fileOrRawString.

Let's also pass some file metadata for both put() and putString() to make sure that is getting carried on as well:

var metadata = { contentType: 'image/jpeg', }; if (isPutString) { task = storage.putString(fileOrRawString, 'raw', metadata); } else { task = storage.put(fileOrRawString, metadata); }

Then you'll need to update the tests below which check the arguments passed to put() and putString().

coveralls · 2017-01-12T12:57:17Z

Coverage increased (+3.7%) to 93.716% when pulling ce4f392 on storage into c22ef72 on master.

jwngr · 2017-01-12T17:31:12Z

LGTM. Nice work!

fallais · 2017-01-13T10:56:07Z

Does that mean we can use the storage right now ? :)

davideast · 2017-01-13T23:17:15Z

@fallais Not yet, but soon! I'm writing up docs and then we'll do a release :)

bpkennedy · 2017-01-14T04:34:21Z

Yes, can we? :)

CyrusZei · 2017-01-14T11:23:06Z

This is so perfect, well done all

…

On Sat, 14 Jan 2017 at 05:34, bpkennedy ***@***.***> wrote: Yes, can we? :) — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#865 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE0dz-UVO3pDgrLKaGNFp3ihSn2CQdhVks5rSFBPgaJpZM4KaC--> .

davideast and others added 9 commits August 16, 2016 14:17

feat(storage): Add bindings for Firebase Storage

440db1a

fix(storage): Linting errors

8ab2b7b

Adds protractor storage test

28e7baa

chore(tests): Storage tests

e477f7b

feat(storage): Add tests

2d49d40

feat(storage): .utils tests

82aa1ae

feat(storage): tests

77679c0

feat(storage): Add FirebaseStorageDirective

d2978d9

chore(storage): Merge conflicts

424ee4f

davideast added the type: feature request label Oct 18, 2016

davideast added this to the AngularFire 3.0.0 milestone Oct 18, 2016

davideast self-assigned this Oct 18, 2016

soumak77 mentioned this pull request Nov 8, 2016

$firebaseStorage and fir-storage-src proposal #785

Closed

soumak77 reviewed Nov 8, 2016

View reviewed changes

chore(tests): Include pertinent source files for coverage. MockTask t…

9624b5b

…ests for storage

fix(build): Add scripts to package.json

42a7a3c

davideast added 2 commits November 21, 2016 05:14

fix(tests): npm script for test

56f3c1a

fix(tests): Travis tests

01ea208

davideast mentioned this pull request Nov 22, 2016

Document $firebaseRefProvider #761

Closed

Merge branch 'master' into storage

6cecd43

fix(build): Remove improper char

8bd3a44

chore(tests): Set upload e2e to manual

4cdd779

davideast removed the request for review from katowulf January 9, 2017 12:58

davideast assigned jwngr and unassigned katowulf Jan 9, 2017

jwngr suggested changes Jan 9, 2017

View reviewed changes

jwngr assigned davideast and unassigned jwngr and davideast Jan 9, 2017

davideast added 2 commits January 10, 2017 05:39

chore(tests): Changes per Jacob's comments

4b15969

chore(tests): Remove ES6 syntax

da9a148

davideast assigned jwngr and unassigned davideast Jan 10, 2017

jwngr suggested changes Jan 10, 2017

View reviewed changes

chore(refactor): , , and other comments from Jacob

e584905

jwngr approved these changes Jan 11, 2017

View reviewed changes

chore(tests): Refactor tests per Jacob's comments

ce4f392

davideast merged commit 9881546 into master Jan 12, 2017

davideast changed the title ~~WIP: feat(storage): Firebase Storage for AngularFire~~ feat(storage): Firebase Storage for AngularFire Jan 12, 2017

jwngr deleted the storage branch January 12, 2017 17:31

feat(storage): Firebase Storage for AngularFire #865

feat(storage): Firebase Storage for AngularFire #865

Conversation

davideast commented Oct 18, 2016 • edited Loading

Firebase Storage for AngularFire!

Description

API Reference

Code sample

googlebot commented Oct 18, 2016

googlebot commented Oct 18, 2016

CyrusZei commented Oct 18, 2016

soumak77 commented Nov 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davideast commented Nov 9, 2016

CyrusZei commented Nov 9, 2016

soumak77 commented Nov 9, 2016

CyrusZei commented Nov 16, 2016

jwngr commented Dec 20, 2016

googlebot commented Dec 23, 2016

coveralls commented Jan 6, 2017 • edited Loading

coveralls commented Jan 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 10, 2017

jwngr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 11, 2017

jwngr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 12, 2017

jwngr commented Jan 12, 2017

fallais commented Jan 13, 2017

davideast commented Jan 13, 2017

bpkennedy commented Jan 14, 2017

CyrusZei commented Jan 14, 2017 via email

davideast commented Oct 18, 2016 •

edited

Loading

coveralls commented Jan 6, 2017 •

edited

Loading