feat: add FileSystemFileHandle support #79
Conversation
rolandjitsu
left a comment
rolandjitsu
left a comment
There was a problem hiding this comment.
@tomayac thank you for your contribution. I only have a question and some unit tests would be good to have.
| } | ||
|
|
||
| function fromDataTransferItem(item: DataTransferItem) { | ||
| if ('getAsFileSystemHandle' in DataTransferItem.prototype) { |
There was a problem hiding this comment.
Some unit tests would be nice.
There was a problem hiding this comment.
Not sure how, it seems like this runs tests against jsDOM, which doesn't emulate this API, unless I'm misunderstanding?
There was a problem hiding this comment.
If you check, I've mocked some of the APIs in order to get some unit tests running.
There was a problem hiding this comment.
I looked into it, but it appears to be a rabbit hole. The current tests fail with ReferenceError: DataTransferItem is not defined, which is documented as jsdom/jsdom#2913, and jsdom/jsdom#2913 (comment) suggests a polyfill, but installing it fails with unresolvable dependency issues. YOLO-updating all dependencies also doesn't work. I'm not familiar with Jest and don't have time to learn how to mock stuff with it. If someone else has idle cycles or experience, this would be appreciated. Else, feel free to close the PR if the tests are a required dependency. No hard feelings.
There was a problem hiding this comment.
I can try to have a look as well. As long as there are no regressions, we could probably merge without tests. Let me see what I can find.
I appreciate the contribution @tomayac
Co-authored-by: Roland Groza <[email protected]>
|
Released with #91. |
2 participants
What kind of change does this PR introduce?
Did you add tests for your changes?
I think TypeScript doesn't know about this feature yet:
Property 'getAsFileSystemHandle' does not exist on type 'DataTransferItem'..If relevant, did you update the documentation?
Summary
Dropped files could be edited right away (instead of forcing a download of a copy), which would be a great win for apps that need to modify dropped files, like online image editors. Also see the MDN docs.
Does this PR introduce a breaking change?
If this PR introduces a breaking change, please describe the impact and a migration path for existing applications.
Other information
Sorry, I don't know much TypeScript. This will need someone to teach TypeScript that 'getAsFileSystemHandle' exists on type 'DataTransferItem'. I wasn't sure how to test this in, since the tests fail because of the TypeScript issue.