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

Drop redundant enable web sockets #2516

Merged
merged 1 commit into from Apr 18, 2019

Conversation

Projects
None yet
3 participants
@mcanlas
Copy link
Contributor

commented Apr 16, 2019

The current BlazeServerBuilder enables web sockets by default. Should that really be the case? I feel like opting-in for features is a better interface.

If not, then the following line can be elided.

@rossabaker
Copy link
Member

left a comment

This gives a slightly worse out-of-box experience for web socket users, but removes an unnecessary blaze stage for non-websocket users. I think this is a net win, but I'm not a web socket user. We'll see if anyone objects.

Oops, didn't read carefully.

@rossabaker
Copy link
Member

left a comment

You're saying since the default is true in the builder, this line in the example is not necessary, right?

@mcanlas

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

Per you original comment, how costly is the extra WS stage vs having WS users add one line?

I would guess WS users are a minority.

@rossabaker
Copy link
Member

left a comment

The cost is light and the users are a minority, but I haven't put a number on either.

Maybe it's better to leave it on until there's a demonstrable performance problem.

@ChristopherDavenport ChristopherDavenport merged commit 1fd16cd into http4s:master Apr 18, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.