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

websocket support in dev #150

Merged
merged 3 commits into from
Dec 21, 2021
Merged

websocket support in dev #150

merged 3 commits into from
Dec 21, 2021

Conversation

threepointone
Copy link
Contributor

This resuscitates https://github.com/threepointone/nu-wrangler/pull/19 via @Electroid. This probably needs tests, in a future PR. But it works well!

This resuscitates threepointone/nu-wrangler#19 via @Electroid. This probably needs tests, in a future PR. But it works well!
@changeset-bot
Copy link

changeset-bot bot commented Dec 21, 2021

🦋 Changeset detected

Latest commit: ef64b87

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

This PR includes changesets to release 1 package
Name Type
wrangler 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

@GregBrimble
Copy link
Member

Great, thanks @threepointone and @Electroid! I'll look at working out how to incorporate this into pages dev as well over the holidays.

Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

WebSockets - FTW!

NIT: I think we could make proxy.ts a bit more maintainable by splitting up the bulk of the code into a few more named functions.

"wrangler": patch
---

Websocket support in `dev`
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be great if we could get a bit more content into these changesets. 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

Comment on lines +581 to +583
headers[name] = value
.replaceAll(`https://${host}`, `http://localhost:${port}`)
.replaceAll(host, `localhost:${port}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it OK to mutate the headers object like this?
Would be safer to design this so that the onResponse(headers) returns a new headers object with the appropriate changes?

Copy link
Contributor

Choose a reason for hiding this comment

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

It should be okay. But both onResponse and onRequest could be redesigned to require a return, instead of mutating.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

happy to revisit this in a future PR, might be a little more involved than just the return from the handler

@threepointone threepointone merged commit 3752acf into main Dec 21, 2021
@threepointone threepointone deleted the proxy branch December 21, 2021 17:59
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.

4 participants