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

Add WebIDLs for Filesystem Access API #3327

Closed
wants to merge 1 commit into from

Conversation

Speak2Erase
Copy link
Contributor

@Speak2Erase Speak2Erase commented Feb 23, 2023

IDL, MDN Docs

I'm pretty new to contributing to projects like this, please lmk if I did something wrong!

I followed the basic instructions listed in web-sys's Readme.

As far as I can tell, the bindings are correct. However, I don't really know what I am doing, especially about [Throws], so that assumption could be wrong.

The Filesystem Access API seems to be pretty stable, however some browsers do not support it (i.e. Firefox) so I added it to unstable.

@daxpedda daxpedda self-requested a review May 11, 2023 11:13
@daxpedda daxpedda self-assigned this May 11, 2023
Copy link
Collaborator

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM otherwise.
Needs a rebase though.

Comment on lines +20 to +25
[Throws]
Promise<File> getFile();
[Throws]
Promise<FileSystemWritableFileStream> createWritable(optional FileSystemCreateWritableOptions options = {});
[Exposed=DedicatedWorker, Throws]
Promise<FileSystemSyncAccessHandle> createSyncAccessHandle();
Copy link
Collaborator

@daxpedda daxpedda May 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These functions don't throw, their Promise can reject with an exception, e.g. https://fs.spec.whatwg.org/#dom-filesystemfilehandle-getfile.

See https://searchfox.org/mozilla-central/source/dom/webidl/FileSystemFileHandle.webidl.

Suggested change
[Throws]
Promise<File> getFile();
[Throws]
Promise<FileSystemWritableFileStream> createWritable(optional FileSystemCreateWritableOptions options = {});
[Exposed=DedicatedWorker, Throws]
Promise<FileSystemSyncAccessHandle> createSyncAccessHandle();
Promise<File> getFile();
Promise<FileSystemWritableFileStream> createWritable(optional FileSystemCreateWritableOptions options = {});
[Exposed=DedicatedWorker]
Promise<FileSystemSyncAccessHandle> createSyncAccessHandle();

Comment on lines +42 to +51
[Throws]
async iterable<USVString, FileSystemHandle>;

[Throws]
Promise<FileSystemFileHandle> getFileHandle(USVString name, optional FileSystemGetFileOptions options = {});
[Throws]
Promise<FileSystemDirectoryHandle> getDirectoryHandle(USVString name, optional FileSystemGetDirectoryOptions options = {});

[Throws]
Promise<undefined> removeEntry(USVString name, optional FileSystemRemoveOptions options = {});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above, none of these should throw: https://searchfox.org/mozilla-central/source/dom/webidl/FileSystemDirectoryHandle.webidl#6.

Suggested change
[Throws]
async iterable<USVString, FileSystemHandle>;
[Throws]
Promise<FileSystemFileHandle> getFileHandle(USVString name, optional FileSystemGetFileOptions options = {});
[Throws]
Promise<FileSystemDirectoryHandle> getDirectoryHandle(USVString name, optional FileSystemGetDirectoryOptions options = {});
[Throws]
Promise<undefined> removeEntry(USVString name, optional FileSystemRemoveOptions options = {});
async iterable<USVString, FileSystemHandle>;
Promise<FileSystemFileHandle> getFileHandle(USVString name, optional FileSystemGetFileOptions options = {});
Promise<FileSystemDirectoryHandle> getDirectoryHandle(USVString name, optional FileSystemGetDirectoryOptions options = {});
Promise<undefined> removeEntry(USVString name, optional FileSystemRemoveOptions options = {});

Comment on lines +73 to +75
Promise<undefined> write(FileSystemWriteChunkType data);
Promise<undefined> seek(unsigned long long position);
Promise<undefined> truncate(unsigned long long size);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the spec all three can throw: https://streams.spec.whatwg.org/#writablestream-get-a-writer.
Not sure why Firefox doesn't have it: https://searchfox.org/mozilla-central/source/dom/webidl/FileSystemWritableFileStream.webidl.

Suggested change
Promise<undefined> write(FileSystemWriteChunkType data);
Promise<undefined> seek(unsigned long long position);
Promise<undefined> truncate(unsigned long long size);
[Throws]
Promise<undefined> write(FileSystemWriteChunkType data);
[Throws]
Promise<undefined> seek(unsigned long long position);
[Throws]
Promise<undefined> truncate(unsigned long long size);

Copy link
Contributor

@djozis djozis May 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @daxpedda, I tried this in Chrome with this sample:

    const storage = await window.navigator.storage.getDirectory();
    const fileHandle = await storage.getFileHandle('test', { create: true });
    const writable = await fileHandle.createWritable();
    const writer = writable.getWriter(); // lock the stream
    try {
        console.log(writable.write('x')); // will fail to get a writer internally
    } catch(e) {
        // this won't run, instead the above promise will fail
        console.log("CAUGHT");
        console.log(e);
    }

I know you linked docs about getting a writer for a writablestream - but it seems this write function returns a promise which fails in the case that getting a writer fails due to the stream being locked, rather than throwing directly. Based on that, I think that like the Firefox webidls, that these should not have the [Throws] annotation, because they give a failed promise instead of throwing when a writer can't be obtained for the stream.

What do you think? Am I missing something? :)

I'm happy to submit another PR to correct this if you agree.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created a PR for this, can continue discussion there perhaps: #3435

@daxpedda daxpedda added the waiting for author Waiting for author to respond label May 11, 2023
djozis added a commit to djozis/wasm-bindgen that referenced this pull request May 14, 2023
@djozis
Copy link
Contributor

djozis commented May 14, 2023

Hopefully not a faux pas, but I wanted this too, including the PR feedback, so I made the changes for my personal use, and thought I'd offer it back here: #3427

@Speak2Erase
Copy link
Contributor Author

Oh, tysm! I don't have much time to fix up this PR. I've been meaning to for a few days now.
If you're okay with continuing #3427, I'd be happy to close this one.

daxpedda pushed a commit that referenced this pull request May 15, 2023
Co-authored-by: Speak2Erase <matthew@nowaffles.com>
@daxpedda
Copy link
Collaborator

Closing in favor of #3427.

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

Successfully merging this pull request may close these issues.

None yet

3 participants