GoogleChromeLabs / browser-fs-access

File System Access API with legacy fallback in the browser
https://googlechromelabs.github.io/browser-fs-access/demo/
Apache License 2.0
1.38k stars 84 forks source link

Need to update types #61

Closed tomayac closed 3 years ago

tomayac commented 3 years ago

fileOpen() and fileSave() now either take an options object, or an array of options objects. The types need to reflect this new change.

// New as of v0.20.0
fileOpen([
    {
      description: 'Image files',
      mimeTypes: ['image/jpg', 'image/png', 'image/gif', 'image/webp'],
      extensions: ['.jpg', '.jpeg', '.png', '.gif', '.webp'],
      multiple: true,
    },
    {
      description: 'Text files',
      mimeTypes: ['text/*'],
      extensions: ['.txt'],
    },
]);

or

// Previous behavior
fileOpen({
  description: 'Image files',
  mimeTypes: ['image/jpg', 'image/png', 'image/gif', 'image/webp'],
  extensions: ['.jpg', '.jpeg', '.png', '.gif', '.webp'],
  multiple: true,
});
tomayac commented 3 years ago

People who have touched the types file before: @dwelle @jmrog @nanaian @raid @fvilers. Anyone able to help? I'm personally not familiar enough with TypeScript.

jmrog commented 3 years ago

Sure, I can take a look!

fvilers commented 3 years ago

I'd write something like this:

type FileOpenOptions = {
...
}

fileOpen(FileOpenOptions | FileOpenOptions[]): FileOpenReturnType;
jmrog commented 3 years ago

I'd write something like this:

type FileOpenOptions = {
...
}

fileOpen(FileOpenOptions | FileOpenOptions[]): FileOpenReturnType;

I did roughly this in my commit above. Your review would be appreciated! 😄

jmrog commented 3 years ago

Caught a small bug in that PR, let me update real quick.

jmrog commented 3 years ago

Caught a small bug in that PR, let me update real quick.

Updated now.

jmrog commented 3 years ago

@tomayac Everything in the PR as-is works, but I'm curious about something. Is it the case that, for fileOpen, only the first object in the options array (when it is an array rather than a scalar value) can have a multiple property? Looking at the code, that at least seems to be the case (multiple properties on other options objects are ignored; seemingly, the same is also true for startIn and id). The PR doesn't currently enforce that, though perhaps it could be made to do so (I'm not immediately sure).

tomayac commented 3 years ago

Yeah, it’s a bit ugly, but this way there’s compatibility with the existing API.

jmrog commented 3 years ago

I noted this on the thread for the PR itself, but I'll note it here too: I updated the PR to also enforce specifying certain properties only on the first options object in the array of options (when an array is provided). As I say there, it makes for a nice developer experience (nice autocomplete and diagnostics), but I'm not sure what TypeScript features you want to use (or, put differently, which TypeScript versions you want to support) in the types for this repo.

tomayac commented 3 years ago

Just wanted to thank you again for the great work. Really makes for an awesome developer experience, even for non-TypeScript developers who happen to use a types-aware IDE.