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: dont use node request as body in handleServerFunction #1577

Merged
merged 1 commit into from
Jul 11, 2024

Conversation

katywings
Copy link
Contributor

@katywings katywings commented Jul 11, 2024

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • infrastructure changes
  • Other... Please describe:

What is the current behavior?

The changes from #1521 broke the form parsing in Node, it threw the error [h3] [unhandled] TypeError: Response body object should not be disturbed or locked.

What is the new behavior?

This fixes the handling for Node, yet tries to keep the functional aspects from the edge runtime workarounds #1282 and #1521. I couldn't test this fix on edge runtimes - @supersoniko could you maybe check if your azure preset still works after this fix ☺️?

Copy link

changeset-bot bot commented Jul 11, 2024

🦋 Changeset detected

Latest commit: a1000be

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@solidjs/start Patch

Not sure what this means? Click here to learn what changesets are.

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

@katywings katywings changed the title WIP: fix: dont use node request as body in handleServerFunction fix: dont use node request as body in handleServerFunction Jul 11, 2024
@ryansolid
Copy link
Member

ryansolid commented Jul 11, 2024

Thanks. Hacks on hacks here until I guess we can get the right resolution from Nitro. I was concerned to merge that PR but it had been sitting for months. I tested all the local apps like Notes/TodoMVC and deployed my hackernews to Netlify and didn't see any problems.

Given this breaks Node I need to move on this faster I think and we can worry about Azure if a new issue comes up.

@ryansolid ryansolid merged commit 5997509 into solidjs:main Jul 11, 2024
1 check passed
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