Skip to content

drop the AbortController patch #519

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 1, 2025
Merged

drop the AbortController patch #519

merged 1 commit into from
Apr 1, 2025

Conversation

vicb
Copy link
Contributor

@vicb vicb commented Apr 1, 2025 •
edited
Loading

This patch was added in #441

The AbortController is meant for the client side but somehow ends up in the server bundler. It was safe to remove it - workerd errors when instantiating an AbortController at top level.

After #497, no more Next code is executed at top level so it is safe to remove the patch.

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Apr 1, 2025 •
edited
Loading

⚠️ No Changeset found

Latest commit: d0ca42c

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

@pkg-pr-new pkg.pr.new
Copy link

pkg-pr-new bot commented Apr 1, 2025 •
edited
Loading

Open in StackBlitz

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

commit: d0ca42c

@vicb vicb requested a review from conico974 April 1, 2025 07:30
conico974
conico974 approved these changes Apr 1, 2025
View reviewed changes
Copy link
Collaborator

@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.
We should probably keep a small comment somewhere though about this with the link to the workerd issue

@vicb
Copy link
Contributor Author

vicb commented Apr 1, 2025

LGTM. We should probably keep a small comment somewhere though about this with the link to the workerd issue

I was wondering if we should keep the patch (but not use it) and maybe remove it later.

WDYT?

@conico974
Copy link
Collaborator

Yeah we can keep it for now and remove it once the fix is made in workerd, we could just add a small comment to explain why it's not needed anymore

@vicb vicb requested a review from conico974 April 1, 2025 12:08
@vicb
Copy link
Contributor Author

vicb commented Apr 1, 2025

I update the PR:

  • restored the rule and added comments on the rule
  • stop applying the patch for now

@vicb
Copy link
Contributor Author

vicb commented Apr 1, 2025

Thanks Nico!

@vicb vicb merged commit 99f28d7 into main Apr 1, 2025
7 checks passed
@vicb vicb deleted the vicb/rmAC branch April 1, 2025 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@conico974 conico974 conico974 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@vicb @conico974