Incorrect type for `FileWithPath`'s `path` and `relativePath` properties

[bthall16]

The FileWithPath interface defines both its path and relativePath properties as optional:

file-selector/src/file.ts

Lines 1233 to 1237 in f159a4a

	export interface FileWithPath extends File {
	readonly path?: string;
	readonly handle?: FileSystemFileHandle;
	readonly relativePath?: string;
	}

indicating they could be undefined.

Looking at the implementation of toFileWithPath, however, it looks like there's no code path where either could be undefined. In particular, if file.path isn't already a string, it's set to a variable which is definitely a string by that point. relativePath is unconditionally set to the same variable mentioned above that's definitely a string.

This seems like a bug with the definition of FileWithPath: if neither path nor relativePath can ever be undefined, then marking them as optional isn't accurate.

I came across this as I was trying to figure out which situations could cause path or relativePath to be undefined so I could account for those situations in my web app.

[jonkoops]

I'm working on improving the correctness of the internal types as we still have to many places where things are any (implicit or otherwise). So I will see if I can resolve this as part of that work.

[bthall16]

Thank you for the reply!

[rolandjitsu]

Probably the reason it is that way is because the type is passed around as input too:

file-selector/src/file.ts

Line 1206 in f159a4a

export function toFileWithPath(file: FileWithPath, path?: string, h?: FileSystemHandle): FileWithPath {

At that point, the path could be undefined.

I guess splitting up the input from the output may make sense. Just not sure if that'll break downstream packages w/o making a breaking change.

[jonkoops]

I guess splitting up the input from the output may make sense. Just not sure if that'll break downstream packages w/o making a breaking change.

Good point, we'll have to investigate this. We're doing a major bump for what is on the beta branch anyways, so that might be a good time to do this if we want to.

[bthall16]

Making path non-optional could be a breaking change for someone using FileWithPath as an input type.

I think* it wouldn't be breaking to have a separate interface where path is required if that interface is only used as part of the return type for fromEvent and friends: any existing code written under the assumption path could be undefined | string would still work if path was only ever string.

* wouldn't be surprised if there are cases in the wild where this doesn't hold

[jonkoops]

Agreed. We're bumping a major anyways, so I would like to ride that and make a clean cut. I plan to do a bunch of refactoring and cleanup to familiarize myself with the code base fully before making this change, so we'll see when I get to it.

[alexandre-combemorel]

I'm using react-dropzone to upload element with aws s3 protocol (docs here) and it seems that it trigger an issue of signature on the back end side.
I narrowed done the issue to this line: (here)

: `./${file.name}`;

while it used to be

: file.name;

I don't understand why we are updating the path to a string with relative path?

If I change it back to the actual original name, it works without issue.

[lomonosv]

I have the same issue with ./ at the beginning of the file name.

[rolandjitsu]

@alexandre-combemorel

I don't understand why we are updating the path to a string with relative path?

because we treat it as a relative path and not having the dot is ambiguous . You could switch to the file name, if that's what you're interested in.

But I see how this is causing some inconvenience.