search

jaydenseric / extract-files

A function to recursively extract files and their object paths within a value, replacing them with null in a deep clone without mutating the original value. FileList instances are treated as File instance arrays. Files are typically File and Blob instances.
https://npm.im/extract-files
MIT License
56 stars 23 forks source link

Fix: extract files from class instances #16

Closed Faithfinder closed 3 years ago

Faithfinder commented 3 years ago

So, we've encountered the same problem as described here: https://github.com/jaydenseric/apollo-upload-client/issues/199. Relevant test provided.

We're using classes to describe our forms, and it wasn't a fun debugging session finding out why in one case our file is uploaded normally, but in the other it's not.

Faithfinder commented 3 years ago

I feel like just closing this off is a little hasty. Sure, now that my team knows about the issue, we can write docs and helper methods and create rules etc. But someone else encountering the problem needs to find out about this first.

I've put in the effort to open this pull request because the manifestation of this problem is really unintuitive and I want to spare anyone following me the painful hours of debugging. Let's keep in mind that the problem for both me and the previous person isn't with this library - it's with apollo-upload-client. Let's look at the client code, i.e apollo mutation execution:

//input is an instance of a class
executeMutation({variables: {
 input
}})

This works as expected in most cases, the only thing failing is if one of the props inside is a file upload. And it's failing pretty far down the line, on the server, when you try to access something on the FileUpload. I am fairly certain that the average user of the library won't be thinking about class instances here. They'll first check their form code 10 times, server code, apollo client config.

For the purposes of sending data to the server it really doesn't matter whether it's a class instance or a simple object - they're serialized in the same way (not thinking about potential edge cases now). I wouldn't even think to try making it a simple object here. We only discovered what the problem is by chance - we had similar code on a different form, but there the form went through some processing before submission (const {discardedProp: _, ...rest} = input). We tried this:

executeMutation({variables: {
 input: {...input}
}})

As a last attempt before going on a dive through the source code of apollo-upload-client.

The argument that not enough people would use class instances as we do is valid. But I really pity those that would, I really don't see them understanding where the problem is in a reasonable amount of time..

jaydenseric commented 3 years ago

All good points you make. Maybe, apollo-upload-client could have a custom JSON.stringify replacer at the very end of the process of preparing variables, that throws a helpful error if there is still a file value (File, Blob, ReactNativeFile, etc. instance) in there after file extraction happened. Would you be keen to experiment with this concept for an apollo-upload-client PR?

Faithfinder commented 3 years ago

I'll see what I can do. I don't really have a lot of energy left after work to do open source, but I do like to finish things I started, so I'll at least have a look