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

[rfc] Implement an ObjectUrl wrapper #231

Merged
merged 2 commits into from
Jul 2, 2022
Merged

Conversation

WorldSEnder
Copy link
Contributor

@WorldSEnder WorldSEnder commented Jun 28, 2022

Rationale

The createObjectUrl API is offered by the browser to create URLs from Blobs that are too large to represent as data urls. The resulting url is along the lines of blob:[origin]://[uuid]. There's a counter part revokeObjectURL that releases the URL again. Rust lends itself to an abstraction that automatically revokes the url when it is not need any longer.

The API

A struct ObjectUrl abstracts the allocation of a specific ObjectUrl. It uses reference counting to revoke the Url after the last user drops its use. It can be created from a Blob, File or directly from a web_sys::Blob. A Deref implemention allows access to the stringly-typed url under-the-hood.

Comment on lines 70 to 74
pub fn new(target: &dyn ObjectUrlTarget) -> Self {
Self {
inner: Rc::new(ObjectUrlAllocation::allocate_new_object_url(target)),
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not just have From impls? From<Blob>, From<File>, etc should be fine. I think it's better to use Rust's traits instead of creating our own, unless we have to.

A macro may be used to make implementing traits easier. Rust's orphan rules will ensure that no more traits can be implemented, effectively sealing it

Copy link
Contributor Author

@WorldSEnder WorldSEnder Jul 1, 2022

Choose a reason for hiding this comment

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

For the most part, it's replicating BlobContents. The "overload" here is for the Url::create_object_url method. A potential impl for MediaSource has been omitted because it's deprecated. Should it be added, too?

Copy link
Collaborator

Choose a reason for hiding this comment

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

For the most part, it's replicating BlobContents. The "overload" here is for the Url::create_object_url method.

I didn't know BlobContents also did it. I'd argue that should be changed as well but that's for another PR. From trait can also provide the overload and (imo) is better since it's part of std lib so users are more familiar with it and understand how that overload work. That may also help avoid looking at documentation to see how to construct it since the user can just use Intellisense with the From trait.

A potential impl for MediaSource has been omitted because it's deprecated. Should it be added, too?

Unless someone complains and has a valid use-case, no, let's not add support deprecated APIs.

crates/file/src/object_url.rs Show resolved Hide resolved
@WorldSEnder WorldSEnder requested a review from ranile July 1, 2022 23:26
@ranile ranile merged commit 2629829 into rustwasm:master Jul 2, 2022
@WorldSEnder WorldSEnder deleted the object-url branch July 2, 2022 11:57
@ranile
Copy link
Collaborator

ranile commented Jul 2, 2022

Released in gloo-file v0.2.3 🎉

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.

2 participants