fix: handle getFile returning null and close #106

[rolandjitsu]

What kind of change does this PR introduce?

• Bug Fix
• Feature
• Refactoring
• Style
• Build
• Chore
• Documentation
• CI

Did you add tests for your changes?

• Yes, my code is well tested
• Not relevant

If relevant, did you update the documentation?

• Yes, I've updated the documentation
• Not relevant

Summary

Address #106.

Does this PR introduce a breaking change?

No.

Other information

[coveralls]

Pull Request Test Coverage Report for Build 12030921930

Details

• 9 of 9 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 100.0%

---

Totals
Change from base Build 12009829103:	0.0%
Covered Lines:	93
Relevant Lines:	93

---

💛 - Coveralls

[jonkoops]

I tried to refactor the logic as much as possible without touching any of the parameters or side-effects passed into toFileWithPath(). I also added some type definitions for getAsFileSystemHandle(), which is not yet included in TypeScript due to the experimental nature of the API.

This is what I came up with:

async function fromDataTransferItem(item: DataTransferItem, entry?: FileSystemEntry | null) {
    const handle = await item.getAsFileSystemHandle?.();

    if (handle && isFileHandler(handle)) {
        const file = await handle.getFile();
        file.handle = handle;
        return toFileWithPath(file);
    }

    const file = item.getAsFile();

    if (!file) {
        throw new Error(`${item} is not a File`);
    }

    return toFileWithPath(file, entry?.fullPath ?? undefined);
}

function isFileHandler(handle: FileSystemFileHandle | FileSystemDirectoryHandle): handle is FileSystemFileHandle {
    return handle.kind === 'file';
}

declare global {
    interface DataTransferItem {
        // This method is not yet widely supported in all browsers, and is thus marked as optional.
        // See: https://developer.mozilla.org/en-US/docs/Web/API/DataTransferItem/getAsFileSystemHandle
        getAsFileSystemHandle?(): Promise<FileSystemFileHandle | FileSystemDirectoryHandle | null>;
    }
}

WDYT?

[buenjybar]

WDYT?

This sounds lovely

Thank you 😄

[rolandjitsu]

@jonkoops thanks, but as far as I can see/remember, fromDataTransferItem is called from toFilePromises only, which is called here:

async function getDataTransferFiles(dt: DataTransfer, type: string) {
    // IE11 does not support dataTransfer.items
    // See https://developer.mozilla.org/en-US/docs/Web/API/DataTransfer/items#Browser_compatibility
    if (dt.items) {
        const items = fromList<DataTransferItem>(dt.items)
            .filter(item => item.kind === 'file');
        // According to https://html.spec.whatwg.org/multipage/dnd.html#dndevents,
        // only 'dragstart' and 'drop' has access to the data (source node)
        if (type !== 'drop') {
            return items;
        }
        const files = await Promise.all(items.map(toFilePromises));

so in practice, items that are not files should not make it through.

So it makes me wonder what else is going on there. @buenjybar if you can, please share a bit more about your use case or maybe a codesandbox and the steps to reproduce so we can figure out what's going on.

[jonkoops]

Got it yeah, I was going off a specific section of code without understanding the whole program fully, which seems to have guided me on some assumptions that were more defensive than required. In general I tend to use the types as guidance and do defensive programming where there is a possibility of a gap between the type and implementation.

I'll do some testing to see if I can reproduce this issue.

[rolandjitsu]

Got it yeah, I was going off a specific section of code without understanding the whole program fully, which seems to have guided me on some assumptions that were more defensive than required. In general I tend to use the types as guidance and do defensive programming where there is a possibility of a gap between the type and implementation.

I'll do some testing on Firefox to see if I can reproduce this issue.

No worries. I appreciate all the help! That sounds good, but I still think that if @buenjybar would provide more context, it would be very helpful.

[buenjybar]

Got it yeah, I was going off a specific section of code without understanding the whole program fully, which seems to have guided me on some assumptions that were more defensive than required. In general I tend to use the types as guidance and do defensive programming where there is a possibility of a gap between the type and implementation.
I'll do some testing on Firefox to see if I can reproduce this issue.

No worries. I appreciate all the help! That sounds good, but I still think that if @buenjybar would provide more context, it would be very helpful.

here #108

this is the full project setup that is reproducing a production bug
https://github.com/buenjybar/react-dropzone-issue106/tree/master/my-app

[rolandjitsu]

Got it yeah, I was going off a specific section of code without understanding the whole program fully, which seems to have guided me on some assumptions that were more defensive than required. In general I tend to use the types as guidance and do defensive programming where there is a possibility of a gap between the type and implementation.
I'll do some testing on Firefox to see if I can reproduce this issue.

No worries. I appreciate all the help! That sounds good, but I still think that if @buenjybar would provide more context, it would be very helpful.

here #108

this is the full project setup that is reproducing a production bug https://github.com/buenjybar/react-dropzone-issue106/tree/master/my-app

Thanks for taking the time to provide a reproducible example. We'll look into it.

[jonkoops]

LGTM, let's get this landed.

[rolandjitsu]

LGTM, let's get this landed.

I wonder if we should reject or go to getAsFile? @MathiasWP do you know how you ended up with your issue?

[jonkoops]

You want to do the fallback solution I proposed instead? It would cover all the cases, but it is hard to tell if it would actually resolve the problem. I would agree with your earlier assessment that if getAsFileSystemHandle() doens't return something, then getFile() likely would not either, but it seems a more fragile API than expected.

[MathiasWP]

LGTM, let's get this landed.

I wonder if we should reject or go to getAsFile? @MathiasWP do you know how you ended up with your issue?

Not sure how it was triggered, we tried investigating but couldn't find anything.

IMO it makes sense to fallback to getAsFile, given that getAsFileSystemHandle is still marked as experimental. Could be a browser bug.

[rolandjitsu]

Ok. It sounds like the sensible thing todo is to go for a fallback. Let me add that.

[rolandjitsu]

Btw, I've added support for returning a custom error in that case where either we have null or getAsFile also returns null. It's in feat/custom-err. If it makes sense, I can create a new PR or add it to this PR.

[jonkoops]

@rolandjitsu could you give this one a rebase with master?

[MathiasWP]

Wouldn't this be a safer fallback? Isn't it nice to try getFile if getAsFileSystemHandle does not return the expected data for some other unknown reason?

Suggested change

	const h = await (item as any).getAsFileSystemHandle();
	if (h === null) {
	throw new Error(`${item} is not a File`);
	}
	// It seems that the handle can be `undefined` (see https://github.com/react-dropzone/file-selector/issues/120),
	// so we check if it isn't; if it is, the code path continues to the next API (`getAsFile`).
	if (h !== undefined) {
	const file = await h.getFile();
	file.handle = h;
	return toFileWithPath(file);
	}
	const h = await (item as any).getAsFileSystemHandle();
	if(!h) {
	const file = await h.getFile();
	if(!file) {
	throw new Error(`${item} is not a File`);
	}
	file.handle = h;
	return toFileWithPath(file);
	}

And then you can skip the

	const file = item.getAsFile(); 
    if (!file) {
        throw new Error(`${item} is not a File`);
    }

below, because it will always try getFile?

[rolandjitsu]

if(!h) {
	        const file = await h.getFile();

That will essentially throw an exception because it will evaluate if h is undefined or null. We want the opposite of that. Note that getFile and getAsFile are 2 different APIs that exist on different objects.

[rolandjitsu]

@rolandjitsu could you give this one a rebase with master?

It should already be rebased.

[github-actions]

🎉 This PR is included in version 2.1.2 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[github-actions]

🎉 This PR is included in version 2.1.2 🎉

The release is available on:

• npm package (@release-2.1.x dist-tag)
• GitHub release

Your semantic-release bot 📦🚀