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

[feature] Add support for kv assets. #308

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

armfazh
Copy link
Contributor

@armfazh armfazh commented Apr 13, 2023

It exposes env.asset_key function so filenames can be queried from the "__STATIC_CONTENT" KV namespace.

Previously, names were mangled due to an additional hash appended to the names.
Now, inside the worker handler one can access to favicon.ico as (simplified):

let key = ctx.env.asset_key("favicon.ico");
let icon = ctx.kv("__STATIC_CONTENT")?.get(&key);

This PR depends on:

Once these PRs get merged, CI will pass.


Credits: This code was based on @SeokminHong comment in #54

Copy link
Collaborator

@zebp zebp left a comment

Choose a reason for hiding this comment

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

I'm unsure if this deserves to be merged into the workers-rs framework itself. Like the existing TypeScript kv-asset-handler this would be better served as a standalone library IMO.

That being said, I'm not totally against adding first party support if there's a good reason as to why it should be in the official crate.

worker-sys/src/asset_handler.rs Outdated Show resolved Hide resolved
worker/src/asset_handler.rs Outdated Show resolved Hide resolved
worker-sys/src/ext/asset_handler.rs Outdated Show resolved Hide resolved
@penalosa
Copy link

penalosa commented Jul 4, 2023

@zebp From the wrangler/miniflare side I tend to agree. Use of Workers Sites is discouraged in favour of Pages, and the assets story in Wrangler will be revisited as part of Pages/Workers convergence in the near future. As such, adding additional functionality to Workers Sites is generally not something we want to do, and introducing a new API directly to the workers-rs framework for this seems like a maintenance footgun in the not too distant future.

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.

None yet

3 participants