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 r2 put response encoding #654

Closed
wants to merge 1 commit into from

Conversation

robbertvanginkel
Copy link

@changeset-bot
Copy link

changeset-bot bot commented Aug 11, 2023

⚠️ No Changeset found

Latest commit: bd74aea

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@robbertvanginkel
Copy link
Author

robbertvanginkel commented Aug 11, 2023

The tre branch seems to be where the currently released version of miniflare is? Are there any tests for this behaviour to verify?

I'm not completely sure why encodeResult doesn't work here, but with it I observed that writeResponse for put would get stuck in this loop:

if (response.body) {
for await (const chunk of response.body) {
if (chunk) initialStream.write(chunk);
}

@mrbbot
Copy link
Contributor

mrbbot commented Aug 11, 2023

Hey! 👋 Thanks for the PR! 🙂 I think this may be caused by nodejs/node#48668, which was introduced in Node 20.4.0. It looks like Node 20.3.0 doesn't reproduce this issue. Unfortunately, this fix doesn't properly encode the returned object. If you try something like...

const object = await env.BUCKET.put("key", "value");
return Response.json(object.checksums);

...you'll see checksums is empty, whereas it should always contain an MD5 hash. I've just finished work to port Miniflare's simulators from Node.js to Durable Objects running in workerd. This should also fix the problem, as the issue is in Node.

Tests for this behaviour are here though:

test("put: returns metadata for created object", async (t) => {

They're not currently run on the latest version of Node 20 though. 🙁

@robbertvanginkel
Copy link
Author

Thanks for the response! I was already getting a bit confused as I thought this had worked on wrangler v3 before but after returning to a project after a while it had suddenly stopped. Couldn't get it to work by downgrading wrangler/miniflare but didn't think node would have been the issue...

With my node fixed to the LTS version the old code seems to work again, I'll close this out and lets wait for the node fix then.

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