Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should choose always return an array? #25

Closed
mkruisselbrink opened this issue Aug 28, 2018 · 12 comments · Fixed by #185
Closed

Should choose always return an array? #25

mkruisselbrink opened this issue Aug 28, 2018 · 12 comments · Fixed by #185
Milestone

Comments

@mkruisselbrink
Copy link
Contributor

In the current strawman proposal the choose style API returns a single handle if multiple is false (or not specified), and returns a sequence if multiple is true (even if in that case only a single file was selected). Would it be more/less confusing if the API always returned an array?

@inexorabletash
Copy link
Member

inexorabletash commented Aug 29, 2018

Some developers will find the "sometimes returns single, sometimes returns array" pattern confusing and expect that if multiple is true but the user selects a single file, a single will be returned. (Because developers don't read specs.)

Is having a separate chooseMultiple() method too crazy?

(Which is to say: having simple choose() with multiple false return an array seems like giving the common case poor ergonomics.)

@DanielHerr
Copy link

I don't think people would expect the return type to be different based on how many files were selected, as that would require additional code to handle both cases. In my experience using the chrome.fileSystem api, the correlation of the return format to the multiple option is sufficiently clear.

@foolip
Copy link
Member

foolip commented Nov 20, 2019

I came here to file a similar issue, which is that the mismatch of plural chooseFileSystemEntries() being able to return a single FileSystemHandle looks unusual.

Two options in addition to the status quo:

  • two methods, one for a single entry and one for multiple
  • a single method that always returns an array

I think always returning an array might be my favorite, but this is bikeshedding for sure.

@mkruisselbrink
Copy link
Contributor Author

Copy-pasting my comment from #146 before I realized it was a duplicate of this issue:

I have no strong opinions on naming here. Originally I had chooseFileSystemEntries always return its results as an array, even if multiple was set to false. But always having to extract an item from that array if you're only expecting a single item anyway seemed annoying, hence why it was changed to not wrap the result in an array if multiple is false. I do agree that that resulted in somewhat unfortunate naming.

As an aside, I'm not a big fan of the chooseFileSystemEntries name at all, perhaps we can side-step this issue with a completely different name? showFilePicker (but what about directories?), or something else? Of course that would still leave the awkwardness that the return type of the method is determined by a passed in option (but that is also true for file vs directory).

It does seem like it might be cleaner to have separate methods with well-defined return types instead. One extreme might be to have 3 (or 4) different methods for all the different possible return types, i.e. separate chooseFile, chooseFiles and chooseDirectory methods (the 4th would be chooseDirectories, but at least chrome doesn't actually support that today; {type: 'open-directory', multiple: true} actually only lets you pick a single directory, the result just gets wrapped in an array).

@domenic
Copy link
Contributor

domenic commented Apr 13, 2020

My feedback:

  • Varying return types is a smell, so I'd prefer to avoid that, either by always returning an array or by splitting into multiple methods. Note that destructuring a single-item array is not that bad: const [handle] = await chooseFileSystemEntries(...). This might be helped if you renamed multiple to allowMultiple; that way it feels like you're always getting "some" entries, but only sometimes is the user allowed to select multiple entries.

  • I lean toward four separate methods, because I cannot imagine a situation in which the code will need to programmatically vary which functionality it calls. I.e. I cannot imagine a scenario where you do chooseFileSystemEntries({ multiple: multipleDeterminedElsewhere, type: typeDeterminedElsewhere }); I can only imagine scenarios where you use constants for those option values. In which case the separate methods are comparatively nice and straightforward.

@jimmywarting
Copy link

jimmywarting commented Apr 14, 2020

What if we instead replaced multiple/allowMultiple with limit?

limit=1
limit=5
limit=infinity

You always get an array and you get a new feature out of it. 1 or infinity could be the default.

@mkruisselbrink
Copy link
Contributor Author

I don't think a limit is something system provided file pickers usually support. Of course we could require browsers to implement their own file pickers, but that still would result in something that behaves in a way users might not be familiar with.

I think I'm leaning towards separate methods as well. That also has the benefit that its clearer which options are applicable to which methods (i.e. "accepts" only for file dialogs).

@cynthia
Copy link

cynthia commented May 27, 2020

@kenchris and I discussed during our VF2F - and we both concluded that polymorphism in this context is an interesting (in an en_UK sense) design decision. Our two cents would be to provide two separate methods or skip the singular case optimization and return a sequence with the cardinality of one and provide a single method.

(Both of us prefer a single method which returns a sequence, but the choice is up to you.)

@kenchris
Copy link

Also in the multiple files chooser, the user can end up just selecting a single file which would then just return a sequence of one single file.

If the dev knows that there will ever only be one file selected, then they can just do files[0] or similar, and if they are prepared to handle multiple files, that code will also work for a single file.

This is also consistent with input type=file

@Anoesj
Copy link
Contributor

Anoesj commented May 28, 2020

Always returning an array, even if multiple: false, makes sense to me. In the end, the user could simply use a destructuring assignment, to prevent writing files[0]in their code. Visually, that'd be some beautiful code IMO 😃.

const [selectedFile] = await window.chooseFileSystemEntries({
  type: 'open-file', 
  multiple: false,
});

mkruisselbrink added a commit that referenced this issue Jun 10, 2020
Rather than having one method do three different things, just have
separate methods for all three options.

This fixes #170, fixes #134, fixes #133 and fixes #25.
mkruisselbrink added a commit that referenced this issue Jun 10, 2020
Rather than having one method do three different things, just have
separate methods for all three options. Also aligns (mostly) with
the file handling API in how accepted file types are specified.

This fixes #170, fixes #134, fixes #133 and fixes #25.
mkruisselbrink added a commit that referenced this issue Jun 17, 2020
* Change the API surface for getting native file system handles.

Rather than having one method do three different things, just have
separate methods for all three options. Also aligns (mostly) with
the file handling API in how accepted file types are specified.

This fixes #170, fixes #134, fixes #133 and fixes #25.
@bradisbell
Copy link

One extreme might be to have 3 (or 4) different methods for all the different possible return types, i.e. separate chooseFile, chooseFiles and chooseDirectory methods (the 4th would be chooseDirectories

@mkruisselbrink On the web developer side, this API would not be desirable. It is strongly desirable to be able to select files and directories simultaneously. It also seems to make sense to have a single consistent API, with a consistent return type.

@mkruisselbrink
Copy link
Contributor Author

It is strongly desirable to be able to select files and directories simultaneously.

What do you mean with this? You want to be able to show a dialog that lets the user select both files and directories? That is not something most systems native file/directory pickers currently support, and I haven't really heard of any use cases that would need that either. Usually opening a file or opening a folder are very separate flows in applications that I am familiar with. Could you elaborate a bit more with what use cases this would allow?

It also seems to make sense to have a single consistent API, with a consistent return type.

The major reason for splitting this up in multiple methods is to have methods with consistent return types. I.e. not have one method where the return type changes depending on the passed in arguments. With separate methods the return type of each method is always the same, making it much easier for static type checkers to validate code being valid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants