refactor!: split file handles into seperate function #126
Conversation
Signed-off-by: Jon Koops <[email protected]> BREAKING CHANGE: Arrays of `FileSystemFileHandle` are now handled in a dedicated `fromFileHandles()` function.
Pull Request Test Coverage Report for Build 12412031215Details
💛 - Coveralls |
|
@rolandjitsu could I get your input on this PR? If you are enjoying the holidays don't worry, this can all certainly wait until next year. |
Sorry for the late reply. Gonna have a look at this now. |
| } else if ( | ||
| Array.isArray(evt) && | ||
| evt.every((item) => "getFile" in item && typeof item.getFile === "function") | ||
| ) { | ||
| return getFsHandleFiles(evt); |
There was a problem hiding this comment.
This would be a breaking change. I imagine it might affect quite a few downstream clients.
It would be good if the commit specifically mentions that (it's currently not explicit that fromEvent doesn't handle lists of file handles).
This will break the default behaviour of react-dropzone as well since it uses fromEvent as the default getFilesFromEvent .
There was a problem hiding this comment.
It might make sense to keep this func as it was and call the new func here to avoid breaking the behaviour of react-dropzone.
There was a problem hiding this comment.
Since we're already shipping several breaking changes on the beta branch I figure this is the best time to make any API changes.
It would be good if the commit specifically mentions that (it's currently not explicit that
fromEventdoesn't handle lists of file handles).
Sure, how would you like me to do so? I can add this to the BREAKING CHANGE: section of the commit description, that should add it to the release notes. Additionally, I intend to write a migration guide for users after the dust has settled on the beta branch, which will likely be a little markdown document linked from the README.
WDYT?
|
@rolandjitsu can we merge this one as-is, or would you like me to make further changes? I have some other work lined up that I'd like to get in as well. |
What kind of change does this PR introduce?
Did you add tests for your changes?
If relevant, did you update the documentation?
Summary
Removes the handling of
FileSystemFileHandlearrays from thefromEvent()function, and moves it to a dedicatedfromFileHandles()function purposefully made for handling this data type. This reduces the need to handle polymorphic input and narrow down what the exact type should be infromEvent(). Additionally, this allows bundlers to more effectively tree-shake the module in case the other code is never used. The JSDoc on the function itself has also been improved with additional details for the consumer.Does this PR introduce a breaking change?
Yes, arrays of
FileSystemFileHandleare now handled in a dedicatedfromFileHandles()function. Users that were previously usingfromEvent()will now have to use this dedicated function instead.Other information
This also removes the notice that this feature is experimental, promoting
fromFileHandles()to a stable API in the next major.