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

Make socket available in clients #30

Merged
merged 12 commits into from
Jun 23, 2013

Conversation

MarkBennett
Copy link
Contributor

A first pass at making Socket and Message available to the client. This will streamline the creation of client/server WebSocket apps.

I've written it so the use of Message in the client is optional but supported. I'd really like feedback on the API if you've got some.

This also starts adding tests to socket.

@MarkBennett
Copy link
Contributor Author

@lvivski thoughts?

@lvivski
Copy link
Owner

lvivski commented Jun 11, 2013

Can you please update the example app to use this new socket library?

@MarkBennett
Copy link
Contributor Author

For sure. I will update the example and the README. I am probably going to add some more tests to but thought I would give you a chance to give some feedback.

One question, do you think adding an optional data parameter to Socket#send makes sense? It should keep the method signature compatible with what's there now but would be more concise than sending socket.send(new Message("test", data: 123)).

@lvivski
Copy link
Owner

lvivski commented Jun 11, 2013

Yes, sendind with socket.send('name', data) is better, than new Message()

@MarkBennett
Copy link
Contributor Author

@lvivski this is still a work in progress but I hit a snag when I discovered that dart:io and dart:html have incompatible implementations of WebSocket. I'm refactoring to accomodate but it's going to take a little longer than I hoped.

Will continue to update this PR.

@MarkBennett
Copy link
Contributor Author

For your reference, here's the bug associated with this issue:

https://code.google.com/p/dart/issues/detail?id=2958

@lvivski
Copy link
Owner

lvivski commented Jun 12, 2013

So I think we'll have to use separate libraries for back-endt and front-end, but it would be great to have one interface for them

@MarkBennett
Copy link
Contributor Author

I have started as you suggest and created an abstract socket class with
front and back end implementations. It will take me a few days to bring
them to parity.
On Jun 12, 2013 12:52 AM, "Yehor Lvivski" notifications@github.com wrote:

So I think we'll have to use separate libraries for back-endt and
front-end, but it would be great to have one interface for them


Reply to this email directly or view it on GitHubhttps://github.com//pull/30#issuecomment-19309421
.

@lvivski
Copy link
Owner

lvivski commented Jun 12, 2013

Ok, I would like to help, but really busy right now :(

@MarkBennett
Copy link
Contributor Author

No worries. I'll continue to hack away on this.

These also come with corresponding tests.
Previously, tests linked in to the /lib folder, exposing some functions
not available to real clients. I would like to re-factor all the test
methods associated with Message into the Socket tests so that Message
does not need to be exposed.
@lvivski lvivski merged commit 70b9a78 into lvivski:master Jun 23, 2013
@MarkBennett
Copy link
Contributor Author

I have further work completed on this. Just need to complete testing the Socket#on method in the browser.

@lvivski
Copy link
Owner

lvivski commented Jun 23, 2013

I've added browser implementation and changes both to use Streams. I didn't see any new commits in your fork, so I've merged this changes

@MarkBennett
Copy link
Contributor Author

Wow. Nice! Will check out when I am back at a computer!
On Jun 23, 2013 10:18 AM, "Yehor Lvivski" notifications@github.com wrote:

I've added browser implementation and changes both to use Streams


Reply to this email directly or view it on GitHubhttps://github.com//pull/30#issuecomment-19876423
.

@lvivski
Copy link
Owner

lvivski commented Jun 23, 2013

I don't know why it's just closed, it's merged actually, but I did a manual merge.

PS: oh, it's the new Github layout, it says merged in the header

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