feat(storage): upload file metadata to existing firestore document

[JonnyBoy333]

Description

This pull requests enhances the uploadFile and uploadFiles methods when using Firestore as your database to store the file metadata. Current behavior is to specify a path to a collection, when a file is uploaded the metadata will create a new document with that data in the specified collection.

In some cases you may already have a document in that collection that you would like to update with the metadata instead of creating a new document. In my use case I have a contacts collection with a list of contact documents and I would like to update an existing contact document with a file download Url from the metadata instead of creating a new document with that information. This PR allows for that functionality by specifying a documentId property in the uploadFile options object. If this property exist the corresponding document will get updated, otherwise a new document will be created.

I've also updated the documentation with this new feature as well as the typescript typings. While updating the docs and typings I noticed some related properties that were missing, like the metadata and metadataFactory properties on the upload options object so I took the liberty of adding typings for those and adding them to the documentation as well. I found that to be important since they impact how my feature works.

Small side note that some of the minor changes in the documentation where made from a pre-commit linting script and not actually something that I changed, hopefully that doesn't cause confusion.

Here's the example I put in the upload examples to see it in action:

Update Firestore Document

If using Firestore for you database and you would like to update a specific document after a file has uploaded you can specify the options.documentId property. In this example the document with id 12345 in the contacts collection will have the fileUrl property updated with the file's download url. More details can be found in the upload file section of the Firebase docs.

const storagePath = 'contacts/sleepygary'
const dbPath = 'contacts'

firebase
  .uploadFile(storagePath, file, dbPath, {
    metadataFactory: (uploadRes, firebase, metadata, downloadURL) => {
      return { fileUrl: downloadURL }
    },
    documentId: '12345'
  })
  .then(() => {
    console.log('File uploaded successfully')
  })

Check List

• All tests passing
• Docs updated with any changes or examples if applicable

Relevant Issues

#665

[codecov]

Codecov Report

Merging #936 into master will decrease coverage by 0.54%.
The diff coverage is 25.00%.

@@            Coverage Diff             @@
##           master     #936      +/-   ##
==========================================
- Coverage   88.87%   88.33%   -0.55%     
==========================================
  Files          29       29              
  Lines         791      797       +6     
==========================================
+ Hits          703      704       +1     
- Misses         88       93       +5     

[prescottprue]

Nice work! Wondering if we should use set with merge instead of update here? Or at least off the option?

[JonnyBoy333]

That's an interesting thought. Using set with the merge property functions the same as update except that in addition to updating properties on an existing document it will create a new document if the documentId does not already exist. I'm thinking set with merge should just be the default. This gives the flixibility of being able to create and update documents in bulk and puts the responsibility on the developer to make sure the documentId exists if they don't want a new document created.

I could add another option property but it kind of seems like if you're smart enough to set a flag to fail if your documentId doesn't already exist you're probably smart enough to have filtered it out of your update in the first place 😉.

I'm going to make this change.

[JonnyBoy333]

I just noticed today that the updateProfile method has a useSet option that is probably what you're talking about here, maybe I will add something like that for consistency's sake.

[prescottprue]

Great work building this out more and separating!

[prescottprue]

Glad you caught this!

[rscotten]

@prescottprue @JonnyBoy333

Adding Blob is now breaking my type check:

  const { uploadTaskSnapshot } = await firebase.uploadFile(
          storagePath,
          acceptedFiles[0],
          undefined,
          {
            name: (file: File) => file.name.replace(/[^\w.\-_]/gi, ''),
          }
        );

Type 'File | Blob' is not assignable to type 'File'.
            Type 'Blob' is missing the following properties from type 'File': lastModified, name ts(2345)

This is because the new type definition is incorrect:

export interface UploadFileOptions {
  name?: string | ((
    file: File | Blob, // should not use the "|" operator
    internalFirebase: WithFirebaseProps<ProfileType>['firebase'],
    uploadConfig: {
      path: string,
      file: Either<File, Blob>,
      dbPath?: string,
      options?: UploadFileOptions
    }
  ) => string)
  ...
}

[JonnyBoy333]

Interesting, I just tried recreating your code and I am not getting any typescript errors. The fact that you are explicitly setting the file to a File type with file: File should let typescript know that it is a File and not a Blob, which allows access to the name property (I do the same thing in my code except for Blob where the name property does not exist).

    const testFile = new File([], 'test');
    const { uploadTaskSnapshot } = await firebase.uploadFile(
      'test/path',
      testFile,
      undefined,
      {
        name: (file: File) => file.name.replace(/[^\w.\-_]/gi, ''),
      }
    );
    // No typescript errors here

The reason for the union type here is because firebases file storage api does support both File and Blob types (as well as Uint8Array). And what type gets passed into the name function is dependent on the type of the second property of the uploadFile function.

There might be a better way to tell typescript that since a File type object was put in the uploadFile function then it can be certain that a File type object will get passed to the name function. I'd have to think about that a little more.

[prescottprue]

Thanks for reaching out - So guessing that should have been Either<File, Blob>?

[rscotten]

@prescottprue @JonnyBoy333 Created a PR with the fix here: #950

[JonnyBoy333]

@rscotten That pull request is what I was thinking of but didn't know how to do myself. Thank you! Now I'm guessing you would write your upload that like this?

const testFile = new File([], 'test');
const { uploadTaskSnapshot } = await firebase.uploadFile<File>( // Adding the type here
  'test/path',
  testFile,
  undefined,
  {
    name: (file) => file.name.replace(/[^\w.\-_]/gi, ''),
  }
);

[rscotten]

@JonnyBoy333 yep! That's correct.

[JonnyBoy333]

@prescottprue Made some updates today per your suggestions. Take a look when you have a chance. Open to feedback.

[JonnyBoy333]

Well, I did update this PR with a useSetForMetadata property on the uploadFiles function. This functions the same way as useSet on the updateProfile function. It defaults to true so you don't have to set it, but if for some reason you want the operation to fail if the documentId is not found you can set this to false and have that functionality. I ran some tests and made sure this is functioning as intended.

I do apologize that I'm not fluent at writing tests in mocha yet. I was hoping I could expand on existing tests around this but all the current ones seem to be around the realtime db and not firestore. I hope this doesn't block this PR.

Otherwise this PR should be ready to go with all your suggestions in place.

[prescottprue]

Thanks for all of the work! Going to merge and release as part of v3.5.0