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

Change the remove operation to be part of a directory handle. #55

Merged
merged 1 commit into from
Jun 4, 2019

Conversation

mkruisselbrink
Copy link
Contributor

Before each handle had a remove method that would remove the entry
represented by the handle. This changes it to instead have a removeEntry
method on a directory handle that will remove a child of that directory.

This means it is no longer possible to delete an individual file or
directory without having a handle to its parent, but otherwise feels
like a slightly nicer API shape to me.

This fixes#54

Copy link
Collaborator

@pwnall pwnall left a comment

Choose a reason for hiding this comment

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

LGTM w/ one nit.

Thank you for the quick follow-up on the issue!

@@ -291,7 +280,7 @@ interface FileSystemDirectoryHandle : FileSystemHandle {

Promise<sequence<USVString>?> resolve(FileSystemHandle handle);

Promise<void> removeRecursively();
Promise<void> removeEntry(USVString name, optional FileSystemRemoveOptions options);
Copy link
Collaborator

Choose a reason for hiding this comment

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

While I personally like remove, the web platform seems to have settled on delete for this operation.

So, WDYT of delete, or deleteEntry?

Also fine to leave the name as-is and let the TAG decide when we submit a review.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'll leave as is for now. Which web platform APIs were you referring to btw? I guess CacheStorage indeed uses delete, but for example DOM has removeChild and removeAttribute

Before each handle had a remove method that would remove the entry
represented by the handle. This changes it to instead have a removeEntry
method on a directory handle that will remove a child of that directory.

This means it is no longer possible to delete an individual file or
directory without having a handle to its parent, but otherwise feels
like a slightly nicer API shape to me.
@mkruisselbrink mkruisselbrink force-pushed the change-remove-method branch from 1a2c82b to bcd36d8 Compare June 4, 2019 23:15
@mkruisselbrink mkruisselbrink merged commit eeaffce into master Jun 4, 2019
@pwnall
Copy link
Collaborator

pwnall commented Jun 4, 2019

I was thinking primarily of WICG/cookie-store#46. I agree we shouldn't block the API improvements in this PR on the name, though. I'll open an issue to track the name and let folks chime in there.

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.

FileSystemDirectoryHandle.removeRecursively() seems redundant with FileSystemHandle.remove()?
2 participants