Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Changes from 18 commits
97fca21
13b8e5e
1e5a833
97e4ae4
161b3d6
cea2ec7
a52a0c6
0418aa8
705ab9c
d2b8610
9ac69ce
f9d12bf
5dd5ac6
514c0fd
1685336
8bdd5fa
b9b16df
6c518da
4551547
2a359bd
66e92ef
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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-placeThere 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
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 ofFileSystemSyncAccessHandle
. 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.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.
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
ofgetSize()
:)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!
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 beNoModificationAllowedError
, 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: