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

Implement Serve feature #128

Merged
merged 3 commits into from
Apr 4, 2016
Merged

Implement Serve feature #128

merged 3 commits into from
Apr 4, 2016

Conversation

Bobo1239
Copy link
Contributor

@Bobo1239 Bobo1239 commented Apr 2, 2016

I'm new to Rust so the code is probably not optimal. Please bear with me. I'm open to suggestions/improvements.

  • In livereload.rs: the websocket server isn't able to handle Ping/Pong messages atm. I tried to make a MessageType.Ping(Box<Message>) but wasn't able to figure out how to deal with the lifetimes.
  • The ComparableSender(Factory) stuff feels pretty hacky... Suggestions?

@Bobo1239
Copy link
Contributor Author

Bobo1239 commented Apr 2, 2016

rust-websocket currently has no option to disable ssl support. We can either try to change that upstream (didn't look at it yet) or switch to another websocket library (e.g. ws-rs).

@azerupi
Copy link
Contributor

azerupi commented Apr 2, 2016

Woah this is awesome!

It will take me some time to review this because I am not very familiar with the dependencies and it's a pretty substantial contribution ;)

rust-websocket currently has no option to disable ssl support. We can either try to change that upstream (didn't look at it yet) or switch to another websocket library (e.g. ws-rs).

That is the cause of the failed tests, right?
Maybe this is something we could ask on users.rust-lang.org to get feedback of people who have already tried some of them.

@Bobo1239
Copy link
Contributor Author

Bobo1239 commented Apr 2, 2016

Ok, ws-rs has ssl support as an optional feature and provides a broadcaster function which allows the whole livereload stuff to be reduced to 20 LOC...

@azerupi azerupi merged commit 6aa6546 into rust-lang:master Apr 4, 2016
@azerupi
Copy link
Contributor

azerupi commented Apr 4, 2016

I just tested this locally and it seems to work very well. It is amazingly useful and will surely be a huge time saver for a lot of people! 👍

There is just one small thing, when you stop serving, the code for the web-socket is still present in the rendered files. It's really no biggie and it can be resolved by just doing a mdbook build, but improving this could be an idea for a future improvement 😉

Anyway, very good job with this

@jgillich
Copy link

@azerupi Can you push an update to crates.io? :)

@azerupi
Copy link
Contributor

azerupi commented Apr 13, 2016

Yes I'll do that right now :)

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