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

Documentation for the 3.x migration #358

Closed
jakemac53 opened this issue May 10, 2024 · 12 comments · Fixed by #363 · May be fixed by #361
Closed

Documentation for the 3.x migration #358

jakemac53 opened this issue May 10, 2024 · 12 comments · Fixed by #363 · May be fixed by #361

Comments

@jakemac53
Copy link
Contributor

I am trying to migration package:build to web_socket_channel 3.x (per 3678), but I am confused as to how to do so.

The changelog only tells you of the breakages (in my case, a removed constructor), but not what I am supposed to use now.

If you go to the WebSocketChannel docs, it still tells you to use the old constructor, which no longer exists (although, dartdoc will link you to it so I guess maybe it exists in some sense, but it isn't directly callable).

What is the replacement for WebSocketChannel.new?

@jakemac53
Copy link
Contributor Author

jakemac53 commented May 10, 2024

In case it is useful, we are using this in testing, with our own manual stream channels (essentially mocking out the actual connection)

@brianquinlan
Copy link
Contributor

What is the replacement for WebSocketChannel.new?

They idea was to simply remove the WebSocketChannel constructor. Let me look at your tests.

@brianquinlan
Copy link
Contributor

If I understand the tests, the idea is that you have two WebSocketChannels that are connected to each other as client and server.

The test writes to the server WebSocketChannel.sink and expects the results to appear in the client WebSocketChannel.stream.

Would it then make sense sense to have a test helper like (WebSocketChannel, WebSocketChannel) createFakes() that returns a client and a server web socket that are connected like I described?

image

@jakemac53
Copy link
Contributor Author

To be perfectly honest, this code is quite old so I have little recollection, but my read is the same as yours generally 🤣 .

Would it then make sense sense to have a test helper like (WebSocketChannel, WebSocketChannel) createFakes() that returns a client and a server web socket that are connected like I described?

I think so, yes. But the old API was more general purpose which was kind of nice (I don't think it's possible for me to implement createFakes myself, which ideally I could?).

I also don't know how much of the deleted code would have to be brought back...

@jakemac53
Copy link
Contributor Author

In general, some mechanism for creating a mocked out web socket channel is basically what I am asking for though, where there is no real connection under the covers, just streams/sinks which I control.

@brianquinlan
Copy link
Contributor

I think that you can create those fakes yourself but it would make more sense for those to be part of package:web_socket_channel.

I don't want to resurrect the deleted code. The deleted code was a modified copy of dart:io's WebSocket implementation for 7 years ago that has not undergone active maintenance. Let's see if the fakes solve the problem ;-)

@brianquinlan
Copy link
Contributor

I have a fake implementation that allows the test to pass locally. The github automation seems to have unrelated failures.

I'd suggest moving these fakes to package:web_socket_channel

Any reason not to do that? @natebosch

@natebosch
Copy link
Member

Any reason not to do that? @natebosch

It doesn't look like it would add any new dependencies, so I can't think of a strong reason not to.

I do wonder whether it's actually necessary - the intention on WebSocketChannel is to act as a StreamChannel. Is there some place we should be loosening an argument type so that we can implement the fake in the test more naturally?

@natebosch
Copy link
Member

Or alternatively, if package:web_socket deserves a fake implementation, we could consider migrating to that package sooner, since that's the long term goal.

@natebosch
Copy link
Member

Migrating to StreamChannel has a slight hiccup, but I think migrating to WebSocket in place of WebSocketChannel looks feasible. @brianquinlan - WDYT about spending the effort on a fake for WebSocket?

@brianquinlan
Copy link
Contributor

Yeah, I'll make a fake for WebSocket.

@lishaduck
Copy link

This was unintentionally closed by #363, but it also depends on #361.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants