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

Fix + update examples #173

Merged
merged 5 commits into from
Dec 16, 2024
Merged

Fix + update examples #173

merged 5 commits into from
Dec 16, 2024

Conversation

vicb
Copy link
Contributor

@vicb vicb commented Dec 14, 2024

See cloudflare/workerd#3245 for details/rationale about the fix.

Updated the examples.

The blog and commerce examples fails, I'll create separate issues for them.
-> See #174 and #175, both added to the board.

Copy link

changeset-bot bot commented Dec 14, 2024

⚠️ No Changeset found

Latest commit: 5480e77

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.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

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

Copy link

pkg-pr-new bot commented Dec 14, 2024

Open in Stackblitz

pnpm add https://pkg.pr.new/@opennextjs/cloudflare@173

commit: 5480e77

vicb added 5 commits December 15, 2024 08:08
Use ReadableStream.from instead of Readable.toWeb
The latter triggers a "This ReadableStream is disturbed" error
See cloudflare/workerd#3245
There is a "[unenv] fs.readdirSync is not implemented yet!" error to investigate
Copy link
Contributor

@conico974 conico974 left a comment

Choose a reason for hiding this comment

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

LGTM

init.body = __cf_stream.Readable.toWeb(init.body);
}
// https://github.com/cloudflare/workerd/issues/3245
body: init.body instanceof __cf_stream.Readable ? ReadableStream.from(init.body) : init.body,
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this fail if init.body is a Readable from unenv ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the top of my head I would say no.
It works with Node (/undici) only because a Readable is async iterable.
And we moved away from unenv Readable because it does not implement async iterator.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking about user code actually, since this is overriding the global Request, user code or one of their lib could be using unenv or similar and instanceof would be false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, got it now.

There should be no more instances of unenv Readable after unjs/unenv#363 (cloudflare/workerd#3245 has some details). Before that it was still possible to create an unenv Readable via the http module.

@vicb vicb merged commit 132b75e into experimental Dec 16, 2024
2 checks passed
@vicb vicb deleted the local/exp branch December 16, 2024 07:35
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.

3 participants