search

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

improve and deduplicate types #15

Closed dwelle closed 4 years ago

dwelle commented 4 years ago

As discussed, I DRYed out the docs by moving them to the type file, and referenced them in JSDoc for ease of use. I've also fixed some types that were supposed to be optional.

I'd say let's migrate to TS, but I have no experience in using TS with ESM packages.

tomayac commented 4 years ago

As you are de-DRY-ing, I also noticed the file names are DRY. The folder has the info we need, no need to repeat it in the file names again:

/src/legacy/file-open-legacy.mjs → /src/legacy/file-open.mjs

Do you want to take care of this in the PR, too?

tomayac commented 4 years ago

Re: https://github.com/GoogleChromeLabs/browser-nativefs/pull/15/commits/1dc49c4ff10f72ccb3cddc6c13daa078f5f31910: The same applies for "nativefs" in the file names. Can you rename those, too?

dwelle commented 4 years ago

Missed. Done.

tomayac commented 4 years ago

Thanks :-) Getting there! While reviewing, I realized the isFile method is no longer there, it's now a kind attribute. Could you do a sanity check of the rest of the types, too?

dwelle commented 4 years ago

Will do later today.

tomayac commented 4 years ago

Will do later today.

Thanks a ton! 👏

dwelle commented 4 years ago

I realized the isFile method is no longer there

It's still there for the OT, right? Either way, it's not shipped yet so do we want to update the interface yet? Anyway, I'll finish this tomorrow as I got held up in other work.

tomayac commented 4 years ago

I realized the isFile method is no longer there

It's still there for the OT, right? Either way, it's not shipped yet so do we want to update the interface yet? Anyway, I'll finish this tomorrow as I got held up in other work.

It's live as of Chrome 86. Not sure if there is a way to express that it can expose either the one, or the other API shape.

dwelle commented 4 years ago

I went with using only the latest spec.

Unrelated: why does the library export a demo function imageToBlob?

tomayac commented 4 years ago

I went with using only the latest spec.

Thanks, this looks good.

Unrelated: why does the library export a demo function imageToBlob?

It's needed in the demo and may come in handy as a general convenience function more people would use.

dwelle commented 4 years ago

may come in handy as a general convenience function more people would use.

That doesn't sound like too strong an argument, it's not even in the readme, and I'd not bloat the library by unrelated util helpers. Just my 2 cents :).

tomayac commented 4 years ago

may come in handy as a general convenience function more people would use.

That doesn't sound like too strong an argument, it's not even in the readme, and I'd not bloat the library by unrelated util helpers. Just my 2 cents :).

That's fair. I'll move it to the demo folder and remove if from the lib in a separate PR.