Add FileSystemHandle::remove() method #283
Conversation
|
@mkruisselbrink PTAL? Particularly at the algorithm portion. Also let me know if you have more details I should add to the description. Thanks! |
index.bs
Outdated
| 1. Attempt to remove |entry| from the underlying file system. | ||
| 1. If |entry| is a [=directory entry=]: | ||
| 1. If |entry|'s path does not exists, [=/reject=] |result| with a | ||
| {{NotFoundError}} and abort |
There was a problem hiding this comment.
These errors aren't linked properly. Not sure the best way to describe these
There was a problem hiding this comment.
https://heycam.github.io/webidl/#idl-DOMException-error-names lists the error names that exist on the platform (and technically everywhere in this spec I wrote things like reject with a {{NotAllowedError}}, that should really say reject with a {{"NotAllowedError"}} {{DOMException}} or something like that).
So the other error types below are not error types that actually exist, so presumably isn't what our draft implementation does either?
There was a problem hiding this comment.
Also I would probably just use similar language as the current language for removeEntry (or ideally make both of them more precise about the errors that can be thrown under which circumstances, I'm not terribly happy with how things currently are defined there either but being consistent is probably better than having completely different language in both places. Ideally for handles in the origin private file system we should be able to exactly define the behavior in terms of concepts defined in this spec).
There was a problem hiding this comment.
Sorry, I should have been more clear here. {{NotFoundError}} is not an issue, but {{NotADirectoryError}}, {{NotAFileError}}, and {{NotEmptyError}} are. These are file errors [1], not DOM exceptions. These are converted to DOMExceptions [2], but some information is lost in translation. The first one here is particularly bad:
FILE_ERROR_NOT_EMPTY_ERROR => DOMExceptionCode::kInvalidModificationError
FILE_ERROR_NOT_A_FILE => DOMExceptionCode::kTypeMismatchError
FILE_ERROR_NOT_A_DIRECTORY => DOMExceptionCode::kTypeMismatchError
The DOMException translations listed here are what should tentatively go in the spec, but there's a TODO [3] for supporting custom messages which are more applicable. While that would be useful it doesn't seem like it should be a blocker here?
[1] https://source.chromium.org/chromium/chromium/src/+/main:base/files/file.h;l=88
[2] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/fileapi/file_error.cc;l=145
[3] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/file_system_access/file_system_access_error.cc;l=62
| :: Attempts to remove the entry represented by |handle| from the underlying file system. | ||
|
|
||
| For files or directories with multiple hardlinks or symlinks, the entry which is deleted | ||
| is the entry corresponding to the path which was used to obtain the handle. |
There was a problem hiding this comment.
@pwnall This text was snagged from your comment here. Let me know if this needs more elaboration #214 (comment)
index.bs
Outdated
| @@ -262,6 +262,8 @@ interface FileSystemHandle { | |||
| readonly attribute FileSystemHandleKind kind; | |||
| readonly attribute USVString name; | |||
|
|
|||
| Promise<void> remove(); | |||
There was a problem hiding this comment.
I wonder if we should have a "recurse: true/false" option analogous to the one removeEntry has. For files it wouldn't matter if you set it or not, but for directories it would make the difference between deleting the directory even when not empty, or only allowing deletion of empty directories.
There was a problem hiding this comment.
You're right, it would be odd to only allow recursive removals via removeEntry. I'll prototype this.
index.bs
Outdated
| 1. Attempt to remove |entry| from the underlying file system. | ||
| 1. If |entry| is a [=directory entry=]: | ||
| 1. If |entry|'s path does not exists, [=/reject=] |result| with a | ||
| {{NotFoundError}} and abort |
There was a problem hiding this comment.
https://heycam.github.io/webidl/#idl-DOMException-error-names lists the error names that exist on the platform (and technically everywhere in this spec I wrote things like reject with a {{NotAllowedError}}, that should really say reject with a {{"NotAllowedError"}} {{DOMException}} or something like that).
So the other error types below are not error types that actually exist, so presumably isn't what our draft implementation does either?
index.bs
Outdated
| 1. Attempt to remove |entry| from the underlying file system. | ||
| 1. If |entry| is a [=directory entry=]: | ||
| 1. If |entry|'s path does not exists, [=/reject=] |result| with a | ||
| {{NotFoundError}} and abort |
There was a problem hiding this comment.
Also I would probably just use similar language as the current language for removeEntry (or ideally make both of them more precise about the errors that can be thrown under which circumstances, I'm not terribly happy with how things currently are defined there either but being consistent is probably better than having completely different language in both places. Ideally for handles in the origin private file system we should be able to exactly define the behavior in terms of concepts defined in this spec).
|
If this method is still desirable (I don't remember discussing it, but it looks reasonable), could you please port this PR to https://github.com/whatwg/fs? |
|
This issue has been ported to the new spec: whatwg/fs#9 |
Adds the ability for a file or directory to delete itself.
Currently, it is not possible to remove a file or directory given its handle. You must obtain the handle of the parent directory and call FileSystemDirectoryHandle::RemoveEntry.
This would enable the common use case where you obtain a file handle from showSaveFilePicker, but then decide you don't want to save after all, and delete the file.
Fixes #214
Preview | Diff