refactor!: split file input change event into seperate function
#129
Conversation
Signed-off-by: Jon Koops <[email protected]> BREAKING CHANGE: File input `change` events are now handled in a dedicated `fromChangeEvent()` function.
Pull Request Test Coverage Report for Build 13499636153Details
💛 - Coveralls |
| @@ -1,2 +1,6 @@ | |||
| export { fromEvent, fromFileHandles } from "./file-selector.js"; | |||
| export { | |||
There was a problem hiding this comment.
I do believe that providing a convenience function that runs though each would help users migrate smoothly.
There was a problem hiding this comment.
I am going to rename and refactor fromEvent to something else as well, perhaps we can then evaluate adding it back and see if it is worth to keep it in a deprecated manner?
I have to do some digging as well and see how we're going to go about upgrading this dependency in the main project, so there will likely be a beta branch there as well.
| * @see {@link https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/file|MDN - `<input type="file">`} | ||
| * @see {@link https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/change_event|MDN - HTMLElement: `change` event} | ||
| */ | ||
| export function fromChangeEvent(event: Event): FileWithPath[] { | ||
| if (!(event.target instanceof HTMLInputElement) || !event.target.files) { |
There was a problem hiding this comment.
Could we also check with isObject? Since we don't know what to expect at runtime.
| if (!(event.target instanceof HTMLInputElement) || !event.target.files) { | |
| if (!isObject(event) || !(event.target instanceof HTMLInputElement) || !event.target.files) { |
There was a problem hiding this comment.
I don't think that is needed, we already have a very specific type of Event, so this value should always be correct. Of course users can pass whatever they want if they are not using TypeScript, but then they are still violating the API contract, and it is their responsibility to do this check.
What kind of change does this PR introduce?
Did you add tests for your changes?
If relevant, did you update the documentation?
Summary
This PR is a continuation of #126 and removes the handling of the
changeevent from thefromEvent()function, and moves it to a dedicatedfromChangeEvent()function purposefully made for handling this specific event.This reduces the need to handle polymorphic input and narrow down what the exact type should be in
fromEvent(), in this case also removing the need for this operation to beasync. 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,
changeevents are now handled in a dedicatedfromChangeEvent()function. Users that were previously usingfromEvent()will now have to use this dedicated function instead.Other information
Not applicable.