-
Notifications
You must be signed in to change notification settings - Fork 22.5k
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 content for FileSystemSyncAccessHandle #21815
add content for FileSystemSyncAccessHandle #21815
Conversation
const accessHandle = await draftFile.createSyncAccessHandle(); | ||
|
||
// Get size of the file. | ||
const fileSize = await accessHandle.getSize(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const fileSize = await accessHandle.getSize(); | |
const fileSize = accessHandle.getSize(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thing about this one is that I'm presenting the async versions of these methods first, kind of as the "default", and then mentioning that the sync ones are available later on. So I'd rather leave this as is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd really prefer we match the spec and have all the code snippets say that these methods are sync, perhaps with a note somewhere that they were async on earlier browser versions (you can link whatwg/fs#7). Part of the reason we were okay with this change is that we weren't expecting too many people to be using this API yet. Putting out documentation encouraging use of the async API is counterproductive in that sense.
We see the async version of this interface as a mistake and do not want to further confuse users with async methods on a _Sync_AccessHandle :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I think you've made some fair points here, plus the spec is now updated. So, for each affected page, which discusses those features and includes related code examples, I've updated the code examples to use the sync methods and included a note warning people that some browsers will still implement the incorrect async versions. Let me know you think of the wording.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The wording looks good to me! 👍
Also don't forget to update this await
of getSize()
:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@a-sully Darnit, I've missed some; I'll check all the pages I've touched again, to make sure. Thanks for the heads-up!
files/en-us/web/api/filesystemfilehandle/createsyncaccesshandle/index.md
Outdated
Show resolved
Hide resolved
files/en-us/web/api/filesystemfilehandle/createsyncaccesshandle/index.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Meta question: Should this document the sync (my preference) or the async versions of the AccessHandle methods?
- The code samples throughout use
{"at" : bla }
, which is JSON. - The code samples should use the
{ create: true }
flag, so it's pasteable. draftFile
should maybe better be nameddraftHandle
. We know that getting a handle on what already is a handle is bad (AccessHandles need a better name whatwg/fs#32).
Co-authored-by: Thomas Steiner <[email protected]>
Co-authored-by: Thomas Steiner <[email protected]>
Co-authored-by: Thomas Steiner <[email protected]>
files/en-us/web/api/filesystemsyncaccesshandle/getsize/index.md
Outdated
Show resolved
Hide resolved
…e/index.md Co-authored-by: Jean-Yves Perrier <[email protected]>
…e/index.md Co-authored-by: Jean-Yves Perrier <[email protected]>
Co-authored-by: Jean-Yves Perrier <[email protected]>
Co-authored-by: Jean-Yves Perrier <[email protected]>
Co-authored-by: Jean-Yves Perrier <[email protected]>
Co-authored-by: Jean-Yves Perrier <[email protected]>
Co-authored-by: Jean-Yves Perrier <[email protected]>
Co-authored-by: Jean-Yves Perrier <[email protected]>
Co-authored-by: Jean-Yves Perrier <[email protected]>
Co-authored-by: Jean-Yves Perrier <[email protected]>
Thanks @teoli2003 ! |
You're welcome, thanks to you for documenting this!
Just note that Chrome will make the async → sync changes in version 108 and the spec PR whatwg/fs#55 (review) is approved and just waiting for @annevk to be merged. Maybe (and I say "maybe" in the sense of "I don't actually know what's the right MDN way of doing it) it makes more sense to document it the other way round: sync as the default, stating that an earlier version of the spec had these functions specified as async. |
🤩
I don't disagree with what you are saying in terms of the spec status and the Chrome implementation, but in my experience MDN docs tend to be written more to reflect what is actually supported out there in production browsers. The sync versions will very soon be in the spec, but Chrome still only supports them behind a pref, and Safari and Firefox don't support them at all yet (saying that, I couldn't get this to work in Firefox, even though the functionality looks to be there in the source code). Once there is more support in production browsers (I'd say at least two), then I think we can revisit this and go with sync versions first. @teoli2003 , does my logic here still make sense in terms of MDN's position on documenting experimental things (i.e. reflect the state in production browsers), or would it be preferable to go with the angle of talking about the latest and greatest features by default, even if they are still experimental, and talking about the older alternatives afterwards (even if that is what is mostly supported in production) ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all this work! I'm excited to see this API get better documentation :)
I took a quick pass with some high-level thoughts. Please reach out if I can clarify any questions!
|
||
A {{jsxref('Promise')}} which resolves to a {{domxref('FileSystemSyncAccessHandle')}} object. | ||
|
||
### Exceptions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put up whatwg/fs#57 a while back to try to better specify the exceptions this API throws. You're welcome to use that as a guide to expand this section
That being said, we can't quite guarantee that a particular error code maps to a specific action, since if the underlying OS provides an error when performing a file operation we often just map that to the closest DOMException
(which may be NoModificationAllowedError
, for example, which we'd otherwise prefer to have reserved for file locking errors)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comments @a-sully !
So for this one, I've made a couple of changes to these descriptions based on your linked descriptions (see next commit). That's a really useful issue text! Let me know what you think.
I think defining what exceptions a feature may throw on MDN has always been tricky, especially if, as in this case, the OS may muddy the waters with its own exceptions. To figure out what exceptions to write about, I tend to just read the algorithm for the feature in the spec to see what exceptions are fired at each stage, do a bit of testing to try to observe behavior, and then write about what I find. Is there a better way, or have you got any tips for making this better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understandable! Unfortunately I don't have any other suggestions... Is it reasonable to link to whatwg/fs#57?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is useful to link it, but I'm not sure where for best exposure. For now, I've added a note to the main API landing page "Concepts and usage" section, which links to it and explains what the issue is:
Note: The different exceptions that can be thrown when using the features of this API are listed on relevant pages as defined in the spec. However, the situation is made more complex by the interaction of the API and the underlying operating system. A proposal has been made to list the error mappings in the spec, which includes useful related information.
files/en-us/web/api/filesystemfilehandle/createsyncaccesshandle/index.md
Outdated
Show resolved
Hide resolved
There is also "save" functionality: | ||
|
||
- In the case of the asynchronous handles, use the {{domxref('FileSystemWritableFileStream')}} interface. Once the data you'd like to save is in a format of {{domxref('Blob')}}, {{jsxref("String")}} object, string literal or {{jsxref('ArrayBuffer', 'buffer')}}, you can open a stream and save the data to a file. This can be the existing file or a new file. | ||
- In the case of the synchronous {{domxref('FileSystemSyncAccessHandle')}}, you write changes to a file using the {{domxref('FileSystemSyncAccessHandle.write', 'write()')}} method, then commit the changes to disk using {{domxref('FileSystemSyncAccessHandle.flush', 'flush()')}}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then commit the changes to disk using {{domxref('FileSystemSyncAccessHandle.flush', 'flush()')}}
This makes it sound like it's a mandatory two-step process, whereas you should really only call flush()
if you need the previous writes to be flushed to disk. Otherwise the underlying OS will flush the writes whenever it sees fit (which should be good enough in most cases...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, it seems a bit odd to include FileSystemSyncAccessHandle
under "save" functionality since it provides so much more than just saves (primarily in-place editing, random-access reads and writes). It might be worth explicitly mentioning that writes using this API are in-place
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to answer any clarifying questions if you need help wording this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I've updated the text to
In the case of the synchronous FileSystemSyncAccessHandle, you write changes to a file using the write() method. You can optionally also call flush() if you need the changes committed to disk at a specific time (otherwise you can leave the underlying operating system to handle this when it sees fit, which should be OK in most cases).
I'll also make sure that this is reflected on the specific flush() reference page.
As for including "FileSystemSyncAccessHandle
under "save" functionality", I think this is OK — I think it is clear that we are just talking about the save functionality available on this class, rather than presenting it as the whole point of FileSystemSyncAccessHandle
. I'll make sure that the write-in-place nature is reflected on the actual class ref pages, and earlier on in the main intro.
const accessHandle = await draftFile.createSyncAccessHandle(); | ||
|
||
// Get size of the file. | ||
const fileSize = await accessHandle.getSize(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd really prefer we match the spec and have all the code snippets say that these methods are sync, perhaps with a note somewhere that they were async on earlier browser versions (you can link whatwg/fs#7). Part of the reason we were okay with this change is that we weren't expecting too many people to be using this API yet. Putting out documentation encouraging use of the async API is counterproductive in that sense.
We see the async version of this interface as a mistake and do not want to further confuse users with async methods on a _Sync_AccessHandle :)
files/en-us/web/api/filesystemfilehandle/createsyncaccesshandle/index.md
Outdated
Show resolved
Hide resolved
|
||
A {{jsxref('Promise')}} which resolves to a {{domxref('FileSystemSyncAccessHandle')}} object. | ||
|
||
### Exceptions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understandable! Unfortunately I don't have any other suggestions... Is it reasonable to link to whatwg/fs#57?
files/en-us/web/api/filesystemfilehandle/createsyncaccesshandle/index.md
Outdated
Show resolved
Hide resolved
const accessHandle = await draftFile.createSyncAccessHandle(); | ||
|
||
// Get size of the file. | ||
const fileSize = await accessHandle.getSize(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The wording looks good to me! 👍
Also don't forget to update this await
of getSize()
:)
There is also "save" functionality: | ||
|
||
- In the case of the asynchronous handles, use the {{domxref('FileSystemWritableFileStream')}} interface. Once the data you'd like to save is in a format of {{domxref('Blob')}}, {{jsxref("String")}} object, string literal or {{jsxref('ArrayBuffer', 'buffer')}}, you can open a stream and save the data to a file. This can be the existing file or a new file. | ||
- In the case of the synchronous {{domxref('FileSystemSyncAccessHandle')}}, you write changes to a file using the {{domxref('FileSystemSyncAccessHandle.write', 'write()')}} method, then commit the changes to disk using {{domxref('FileSystemSyncAccessHandle.flush', 'flush()')}}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to answer any clarifying questions if you need help wording this!
@a-sully great, thanks for the further comments! I think I've fixed everything up, but let me know if you've got any more concerns. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A tiny detail that I will commit right away.
* add content for FileSystemSyncAccessHandle * Update files/en-us/web/api/file_system_access_api/index.md Co-authored-by: Thomas Steiner <[email protected]> * Update files/en-us/web/api/file_system_access_api/index.md Co-authored-by: Thomas Steiner <[email protected]> * Update files/en-us/web/api/file_system_access_api/index.md Co-authored-by: Thomas Steiner <[email protected]> * Making fix according to tomayac comment * fixing code examples according to tomayac comments * Update files/en-us/web/api/filesystemfilehandle/createsyncaccesshandle/index.md Co-authored-by: Jean-Yves Perrier <[email protected]> * Update files/en-us/web/api/filesystemfilehandle/createsyncaccesshandle/index.md Co-authored-by: Jean-Yves Perrier <[email protected]> * Update files/en-us/web/api/filesystemfilehandle/index.md Co-authored-by: Jean-Yves Perrier <[email protected]> * Update files/en-us/web/api/filesystemsyncaccesshandle/close/index.md Co-authored-by: Jean-Yves Perrier <[email protected]> * Update files/en-us/web/api/filesystemsyncaccesshandle/flush/index.md Co-authored-by: Jean-Yves Perrier <[email protected]> * Update files/en-us/web/api/filesystemsyncaccesshandle/getsize/index.md Co-authored-by: Jean-Yves Perrier <[email protected]> * Update files/en-us/web/api/filesystemsyncaccesshandle/index.md Co-authored-by: Jean-Yves Perrier <[email protected]> * Update files/en-us/web/api/filesystemsyncaccesshandle/read/index.md Co-authored-by: Jean-Yves Perrier <[email protected]> * Update files/en-us/web/api/filesystemsyncaccesshandle/read/index.md Co-authored-by: Jean-Yves Perrier <[email protected]> * Update files/en-us/web/api/filesystemsyncaccesshandle/write/index.md Co-authored-by: Jean-Yves Perrier <[email protected]> * Respond to a-sully review comments and make some other fixes * more fixes for a-sully comments * Update files/en-us/web/api/file_system_access_api/index.md Co-authored-by: Thomas Steiner <[email protected]> Co-authored-by: Jean-Yves Perrier <[email protected]>
Description
This PR adds MDN content for:
In the current spec,
close()
,flush()
,getSize()
, andtruncate()
are specified as asynchronous — they return promises. Chrome 108 has introduced new synchronous versions of these methods, behind a flag.Specs:
Questions:
Motivation
These features were not documented before, and people want to know about the new features.
Additional details
Tested in Safari and Chrome with these samples:
The Gecko source code contains WebIDL for these features, preffed off by default (see https://searchfox.org/mozilla-central/source/dom/webidl/FileSystemSyncAccessHandle.webidl). I enabled the pref and tried my demos in Firefox, but they didn't work (failed silently), and I'm not sure why.
Other supporting pages and evidence:
Related issues and pull requests