bug(storage): issue uploadFile when using meta information

[reimertz]

It seems we are making an assumption here that FireStore add and Database push resulting in the same returned value. But it seems Firestore returns a snapshot where's database push only returns an error if something went wrong.

react-redux-firebase/src/utils/storage.js

Line 140 in 51ce8db

: firebase // Write metadata to Real Time Database

I assume this also relates to the issues seen in #487.

[prescottprue]

Good find. Feel free to open to a PR if you get a chance, otherwise I will try to get a fix out this weekend.

[reimertz]

Cool!

I'm trying to figure out how this code works. It seems we're doing uploadResultFromSnap after we've pushed fileData using

...

var metaSetPromise = function metaSetPromise(fileData) {
    return useFirestoreForStorageMeta ? firebase.firestore().collection(dbPath).add(fileData) : firebase.database().ref(dbPath).push(fileData);
  };

return metaSetPromise(fileData).then(resultFromSnap);

So I don't understand the reasoning why we want to .then(resultFromSnap).

Is it because we want to be able to dispatch FILE_UPLOAD_COMPLETE? If so, it seems nothing is dependent on that action anyway?

[reimertz]

if I do return metaSetPromise(fileData)//.then(resultFromSnap); all works as intended.

It also seems that uploadRes returns downloadURL. :S

fileMetadataFactory: (uploadRes) => {
    const { downloadURL, metadata: { name, timeCreated, size } } = uploadRes

    return {
      uri: downloadURL,
      name,
      timeCreated,
      size
      
    }
  }

[prescottprue]

Yeah, glad you are finding the same thing. As mentioned in #487, that seems to be happening out of order and will need to be fixed.

[jenphillips]

Thanks for all the work on this library - just started using it a couple weeks ago and it's amazing.

I ran into the same problem described in #480 when trying to upload files, dug around a bit and ended up installing from the v3.0.0-alpha branch to get the latest code adding downloadURL to the file metadata . I ran into an async-related error:

"ReferenceError: regeneratorRuntime is not defined"

Seems like this can be fixed by installing babel/polyfill: babel/babel#5085

I installed that, and also updated the syntax of the arguments passed to fileMetadataFactory. It looks like downloadURL is not part of the default uploadRes object, but comes in separately?

fileMetadataFactory: (uploadRes, firebase, metadata, downloadURL) => {
  const { metadata: { name, fullPath } } = uploadRes
  return {
    name,
    fullPath,
    downloadURL
  }
}

With these changes, the file upload works and successfully stores metadata to Firestore (including the download URL). Commit is here:

jenphillips@e10e47b

Let me know if it is worth opening a PR, or if someone else is already addressing this. Thanks again!

[prescottprue]

@jpierce42 Definitely worth making a PR, thanks for reaching out! We may want to make it against the v2.2.* version that is currently on next (branch and tag).

Surprised to see the regeneratorRuntime issue popup since that should be handled by preset-env as mentioned in the issue you posted, which version of node are you using? Either way, we should address that as well.

[jenphillips]

I was on node v8.11.4. I tried upgrading to 10.10.0 and saw the same error. I will dig a little deeper into the babel issue and see if I can find a better solution. In the mean time, opened a PR against v2.2.0-alpha.2 - if you need any changes just let me know.

[jenphillips]

Looks like the regeneratorRuntime error is also fixed if I add transform-runtime to the main plugins section in .babelrc:

{
  "presets": [
    "react",
    ["env", {
      "targets": {
        "chrome": 52,
        "browsers": ["last 2 versions", "safari >= 7"]
      }
    }]
  ],
  "plugins": [
    "lodash",
    "add-module-exports",
    "transform-object-rest-spread",
    "transform-object-assign",
    "transform-class-properties",
    "transform-export-extensions",
    "transform-runtime"  <---------------------------------
  ],
  "env": {
    ...
  }
}

Previously it was only referenced under the test environment. Is this preferable to manually installing and referencing babel/polyfill? Admittedly not a Babel expert. :) Let me know if so and I can update the PR.

[prescottprue]

This was released in v2.2.0-alpha.2 with the PR from @jpierce42 and associated unit tests.

@jpierce42 I found out that the transform-runtime was needed since I accidentally used async directly in the src (instead of just in tests). After fixing that and switching the babel config back, it seems good. Let me know if that isn't the case for you, and thanks for the PR!