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

Workers Assets Binding #656

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

BrandonDyer64
Copy link
Contributor

@BrandonDyer64 BrandonDyer64 commented Oct 18, 2024

closes #644

I went down a rabbit hole creating an entire new Assets env binding just to find out it's a Fetcher. In fact, you can use assets without this PR by just calling env.get_binding::<Fetcher>("ASSETS").

Testing:

  • Run npx wrangler dev in worker-sandbox
  • Go to /asset/test.txt
  • See TEST in browser
  • Go to /asset/doesnotexist.txt
  • See 404

@BrandonDyer64
Copy link
Contributor Author

BrandonDyer64 commented Oct 18, 2024

Maybe this isn't enough. I'm doing some testing and setting the following in my wrangler.toml will serve the assets, but not expose the binding:

[assets]
binding = "ASSETS"
directory = "./public/"

I get something along the lines of

Your worker has access to the following bindings:
- D1 Databases:
  - DB: anaso (00000000-0000-0000-0000-000000000000)
- Vars:
  - ANASO_JWT_SECRET: "################"

Nothing related to assets.

I'm on wrangler 3.81.0, worker-build 0.1.0, and compatibility_date = "2024-09-19".

Edit: Resolved. The assets binding is working correctly, just not printing out.

@kflansburg
Copy link
Contributor

Sorry that you went in circles on this. I think this looks correct. Have you tried deploying your worker and testing that? I will see if I can reproduce the issue in local development.

@kflansburg
Copy link
Contributor

This worked locally for me. You do need to specify a dummy URL (I think all Fetchers behave this way).

@BrandonDyer64
Copy link
Contributor Author

You do need to specify a dummy URL

Ah, that was it. Adding https://example.com to the beginning of my URL fixed it for me.

@BrandonDyer64
Copy link
Contributor Author

Couple questions/feedback on this:

  1. I don't see the ASSETS binding in the Your worker has access to the following bindings: printout. That seems like a bug, and it's why it took me so long to find a solution. I was chasing ghosts.
  2. What's your recommendation for getting the fetch() response body as bytes? I had to import http_body_util::BodyExt and write this, just to get basic data out:
    • let bytes: Vec<u8> = response.into_body().collect().await?.to_bytes().to_vec();

@kflansburg
Copy link
Contributor

Couple questions/feedback on this:

  1. I don't see the ASSETS binding in the Your worker has access to the following bindings: printout. That seems like a bug, and it's why it took me so long to find a solution. I was chasing ghosts.

  2. What's your recommendation for getting the fetch() response body as bytes? I had to import http_body_util::BodyExt and write this, just to get basic data out:

    • let bytes: Vec<u8> = response.into_body().collect().await?.to_bytes().to_vec();

I think 1. may be a bug in wrangler cc @petebacondarwin

For 2, I assume you are using the http crate feature? I think the main way to consume it is as a stream like you are doing, but I suppose we could add some utility methods to the concrete Body type you get from our Fetcher::fetch.

@BrandonDyer64
Copy link
Contributor Author

we could add some utility methods to the concrete Body type you get from our Fetcher::fetch

I'd love to see an async fn to_bytes() -> Vec<u8> and async fn to_json<T>() -> T on Body if possible. It would make life a lot easier.

@BrandonDyer64
Copy link
Contributor Author

I'm not sure why the tests are failing in the CI. Started happening after my first commit, which just added a single method to Env.

@BrandonDyer64
Copy link
Contributor Author

Can anyone review this?

@kflansburg
Copy link
Contributor

I'm no longer with Cloudflare, so I cannot approve. I think test failures are related to the Rust 1.82 issue here. We may need to pin the Rust version in our CI.

Otherwise, I think this is a good test to add. I would like to see it also tested with http flag enabled, we are trying to keep those two test suites identical and will probably eventually make http the default (or at least that was my original plan).

cc @harrishancock

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.

Introduce Workers Assets binding
2 participants