Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Firebase storage UploadTask callback type requires all callbacks #3158

Closed
nicholaschiang opened this issue Jun 3, 2020 · 2 comments · Fixed by #3224
Closed

Firebase storage UploadTask callback type requires all callbacks #3158

nicholaschiang opened this issue Jun 3, 2020 · 2 comments · Fixed by #3224
Assignees
@schmidt-sebastian
Labels
api: storage

Comments

@nicholaschiang
Copy link

[REQUIRED] Describe your environment

  • Operating System version: Ubuntu 18.04.2
  • Browser version: N/A
  • Firebase SDK version: 7.14.2
  • Firebase Product: storage

[REQUIRED] Describe the problem

Steps to reproduce:

The UploadTask callback type doesn't match what is documented here:

Callbacks can be passed either as three separate arguments or as the next, error, and complete properties of an object. Any of the three callbacks is optional, as long as at least one is specified. In addition, when you add your callbacks, you get a function back. You can call this function to unregister the associated callbacks.

But the UploadTask callback parameter type seems to require all three callbacks (i.e. it needs an object with next, complete, and error fields).

Relevant Code:

So I've called the UploadTask on() method using an object containing the error and complete callbacks as follows:

 ref.put(file).on(firebase.storage.TaskEvent.STATE_CHANGED, {                                                                                                                                                                 
   async complete() {                                                                                                                                                                                                         
     onChange((await ref.getDownloadURL()));                                                                                                                                                                                  
     setValue(`Uploaded ${filename}.`);                                                                                                                                                                                       
   },                                                                                                                                                                                                                         
   error(error: Error) {                                                                                                                                                                                                      
     setErrored(true);                                                                                                                                                                                                        
     setValue(`An error occurred while uploading ${filename}. ${error.message}`);                                                                                                                                             
   },                                                                                                                                                                                                                         
 });

But the UploadTask.prototype.on() type seems to require all three callbacks as I'm getting this Typescript error:

 Argument of type '{ complete(): Promise<void>; error(error: Error): void; }' is not assignable to parameter of type 'Observer<UploadTaskSnapshot, Error> | ((a: UploadTaskSnapshot) => any) | null | undefined'.             
   Property 'next' is missing in type '{ complete(): Promise<void>; error(error: Error): void; }' but required in type 'Observer<UploadTaskSnapshot, Error>'.

And the relevant UploadTask.prototype.on() type is defined in node_modules/firebase/index.d.ts as follows:

   7579     on(                                                                                                                                                                                                               
   7580       event: firebase.storage.TaskEvent,                                                                                                                                                                              
   7581       nextOrObserver?:                                                                                                                                                                                                
   7582         | firebase.Observer<UploadTaskSnapshot>                                                                                                                                                                       
   7583         | null                                                                                                                                                                                                        
   7584         | ((a: UploadTaskSnapshot) => any),                                                                                                                                                                           
   7585       error?: ((a: Error) => any) | null,                                                                                                                                                                             
   7586       complete?: firebase.Unsubscribe | null                                                                                                                                                                          
   7587     ): Function;

Where a firebase.Observer is defined as:

     72   /**                                                                                                                                                                                                                 
     73    * @hidden                                                                                                                                                                                                          
     74    */                                                                                                                                                                                                                 
     75   interface Observer<T, E = Error> {                                                                                                                                                                                  
     76     next: NextFn<T>;                                                                                                                                                                                                  
     77     error: ErrorFn<E>;                                                                                                                                                                                                
     78     complete: CompleteFn;                                                                                                                                                                                             
     79   }

Recommended fix
The fix here should be pretty easy. Just change the parameter in line 7582 of index.d.ts to be a Partial of firebase.Observer as follows:

   7582         | Partial<firebase.Observer<UploadTaskSnapshot>>

This will make all of those callbacks optional. Though note that this is not a perfect solution, as your documentation specifies that at least one callback should be provided:

Any of the three callbacks is optional, as long as at least one is specified.

I'm sure that there's a way to do that with Typescript's built-in utility types though (I just don't want to try to figure it how right now because it's your job haha).
@schmidt-sebastian schmidt-sebastian self-assigned this Jun 9, 2020
@schmidt-sebastian
Copy link
Contributor

@nicholaschiang Thanks for this report. I will take a look and see if we can fix this without breaking the API.

@schmidt-sebastian schmidt-sebastian mentioned this issue Jun 16, 2020
Merged
@schmidt-sebastian
Copy link
Contributor

Pending review: #3224

@Feiyang1 Feiyang1 mentioned this issue Jul 16, 2020
Merged
@firebase firebase locked and limited conversation to collaborators Aug 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@schmidt-sebastian schmidt-sebastian

Labels
api: storage
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Allow undefined callbacks in Storage Observer firebase/firebase-js-sdk
3 participants
@schmidt-sebastian @nicholaschiang @google-oss-bot