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

[WIP] Support for OPFS #841

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

DanielMoss
Copy link
Contributor

Hi!

I was hoping to add support for the OPFS. I've run into two areas where I was hoping to get advice - I'll highlight these in PR comments.

@DanielMoss DanielMoss marked this pull request as draft March 17, 2024 21:08
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This type also defines methods that return async iterators, which don't appear to be supported at the moment in scala-js. It makes sense to me to tackle those methods in a separate piece of work. I've asked about potentially adding support over in the scala-js discord.

Copy link
Member

Choose a reason for hiding this comment

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

* FileSystemFileHandle.createWritable() method.
*/
@js.native
trait FileSystemWritableFileStream extends js.Object {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

MDN

I'm a little lost here at the moment. This should actually extend a WritableStream type, and I started implementing that before realising that there's already a WriteableStream (sp) defined in scala-js-dom. However what's already defined looks quite different to what I'd have expected from the whatwg/MDN spec. I came across #754 which looks related.

Ultimately I'm not sure how to proceed with the facade for FileSystemWritableFileStream when it comes to whether or not to integrate with the existing WriteableStream. I'd appreciate any thoughts or advice.

Copy link
Member

Choose a reason for hiding this comment

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

However what's already defined looks quite different to what I'd have expected

In what way? There are a couple mistakes but it should be the same thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad - definitely should have elaborated in the original comment.

My confusion in primarily that the existing interface in scala-js-dom seems to resemble the spec for WritableStreamDefaultWriter rather than WritableStream. However the current scala-js-dom version also includes a state method that doesn't exist in either public interface (though state is an internal slot for the WritableStream spec, albeit with different potential values).

If we treat WriteableStream as a misnamed WritableStreamDefaultWriter, then it seems clear that an implementation of FileSystemWritableFileStream should not extend WriteableStream. However we have methods like CompressionStream#writable that would then be returning WritableStreamDefaultWriter rather than an actual WritableStream like the spec suggests.

Maybe the spec has evolved over time, or I'm missing a trick with WritableStream = WritableStreamDefaultWriter.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I see. That's a quite a bit more than a couple mistakes 😅 thanks for bringing that to our attention.

Since the name doesn't correspond to anything in the actual DOM maybe we should just deprecate it and start over, with the right names, class/trait designations, and APIs 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, glad to know I'm interpreting the code correctly. I'm still familiarising myself with a lot of this stuff. :)

Open to any ideas you guys have on how to handle this. Maybe it's best if I remove FileSystemWritableFileStream from this PR to simplify things, and we can move discussion of what to do about WriteableStream to a separate issue - maybe #629?

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 this pull request may close these issues.

2 participants