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

feat: support throwing responses in upgrade hook #91

Merged
merged 19 commits into from
Jan 21, 2025
Merged

Conversation

LukeHagar
Copy link
Contributor

@LukeHagar LukeHagar commented Nov 13, 2024

This PR should introduce support for throwing for errors or returning for success from the upgrade handler, all the interfacing with the adapters should remain the same.

@pi0
Copy link
Member

pi0 commented Nov 14, 2024

Seems nice direction! It might take me little longer to jump on proper review.

@LukeHagar
Copy link
Contributor Author

@pi0 Any chance you may be able to take a look at this PR soon?

@LukeHagar
Copy link
Contributor Author

@pi0 Circling back around on this, I see these changes as a pseudo-prerequisite for getting Web Sockets added to SvelteKit.

I'm happy to work on any changes or feedback that needs to be incorporated and would love to get tests running if some exist.

@LukeHagar
Copy link
Contributor Author

@pi0 Can you assist with the logic around the errors? I see that the tests expect ws.Error to be populated on unauthorized response, how is that properly set cross platform?

@pi0
Copy link
Member

pi0 commented Dec 16, 2024

Of course. Applogizes again for delay. I will directly check tests and changes ASAP to move it forward.

@benmccann
Copy link

Thanks @pi0! I hope you had a good new year. We're thankful for any time you're able to spare to take a look at this

@pi0 pi0 changed the title feat: support for throwables feat!: support for throwables Jan 21, 2025
@pi0 pi0 changed the title feat!: support for throwables feat: support for throwables Jan 21, 2025
@pi0 pi0 changed the title feat: support for throwables feat: support throwing responses in upgrade hook Jan 21, 2025
@unjs unjs deleted a comment from codecov bot Jan 21, 2025
@pi0
Copy link
Member

pi0 commented Jan 21, 2025

Thanks again and sorry for the delay. Pushed a few changes:

  • Logic for handling hook return value is unified into hooks.upgrade
  • Added back partial ResponseInit support for backward compatibility with return { headers }
  • If returned Response has ok: false status, it still ends the response also for backward compatibility (discovered from failing tests)

@unjs unjs deleted a comment from codecov bot Jan 21, 2025
@pi0 pi0 merged commit c850a66 into unjs:main Jan 21, 2025
4 checks passed

This comment was marked as off-topic.

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