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 AccessHandle proposal #310

Merged
merged 8 commits into from
Aug 4, 2021
Merged

Add AccessHandle proposal #310

merged 8 commits into from
Aug 4, 2021

Conversation

fivedots
Copy link
Collaborator

@fivedots fivedots commented Jul 2, 2021

This PR adds the proposal document for AccessHandles. AccessHandles are a new proposed surface that allows us to support Storage Foundation API's use cases through the Origin Private File System.

Further details on the origins of this proposal, and alternatives considered, can be found in the following documents: Merging Storage Foundation API and the Origin Private File System, Recommendation for Augmented OPFS.


Preview | Diff

@fivedots fivedots requested a review from rstz July 2, 2021 15:56
AccessHandle.md Outdated Show resolved Hide resolved
AccessHandle.md Outdated Show resolved Hide resolved
AccessHandle.md Outdated Show resolved Hide resolved
AccessHandle.md Outdated Show resolved Hide resolved
AccessHandle.md Show resolved Hide resolved
AccessHandle.md Show resolved Hide resolved
AccessHandle.md Outdated Show resolved Hide resolved
AccessHandle.md Outdated Show resolved Hide resolved
AccessHandle.md Outdated Show resolved Hide resolved
Copy link
Contributor

@rstz rstz left a comment

Choose a reason for hiding this comment

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

LGTM modulo comments

AccessHandle.md Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
AccessHandle.md Outdated

Promise<undefined> truncate([EnforceRange] unsigned long long size);
Promise<unsigned long long> getSize();
Promise<undefined> flush();
Copy link
Contributor

Choose a reason for hiding this comment

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

iirc, in the NativeIO/Storage Foundation API these were all synchronous as well. Could you elaborate on what you learned that made you change the API shape in this newer proposal?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've added an explanation under "New data access surface", could you take a look and tell me if that resolves this comment?


This proposal is focused only on additions to the [Origin Private File
System](https://wicg.github.io/file-system-access/#sandboxed-filesystem), and
doesn't currently consider changes to the rest of File System Access API or how
Copy link
Contributor

Choose a reason for hiding this comment

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

While I know you're focusing on the origin private use cases specifically for this proposal, I think it would be worthwhile to explore what parts of this API could be exposed on arbitrary file handles as well. I believe there are a number of use cases that could be addressed by adding a couple of options to the createAccessHandle methods to slightly tweak its behavior.

Straw-man proposal createAccessHandle({inPlace: true, locking: true}) would match what you've described here, and be only available for origin private files.

On the other hand inPlace: false could then work with the same API surface for arbitrary file handles. While that would of course duplicate some API surface from the existing createWritable method, being able to read what has been written before it is flushed/comitted seems useful, and if this new API will be built on top of some better seekable streams primitive that might be a benefit as well.

Not sure if a non-locking version for the origin private file system makes sense, or is desirable, but for arbitrary files I think it would be useful to be able to create both locking and non-locking access handles.

While all that can probably be added after the fact, we can't really change the defaults for such options easily, so while it might perhaps make sense to initially only support a "inPlace: true, locking: true" mode (and throw/reject for any other options), I don't think that should be the default flags if we actually want to pursue a more unified API.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a good point, I agree.

I've added a "mode" parameter to createAccessHandle, and an open question describing why it's there and what other values it could have. Please take a look and let me know if this covers what you meant!

@fivedots fivedots merged commit c33a96f into main Aug 4, 2021
@fivedots fivedots deleted the access-handle-proposal branch August 4, 2021 14:48
github-actions bot added a commit that referenced this pull request Aug 4, 2021
Add AccessHandle proposal

SHA: c33a96f
Reason: push, by @fivedots

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

4 participants