-
Notifications
You must be signed in to change notification settings - Fork 258
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
Switch over to Tokio ecosystem #105
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know enough about warp to review that. So I'll leave @thedodd to handle that part of it
I've updated to Tokio 1.0 so this PR is ready to be reviewed, I don't foresee any more changes CC: @thedodd, @rakshith-ravi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hamza1311 hey boss, thanks for all of the great work here. I'm actually not done with the review yet, but I wanted to give you some early feedback. I'll finish up the remainder of the PR review soon.
I would definitely love to get this released as part of 0.8 (the next release).
# Conflicts: # Cargo.lock # Cargo.toml # src/build.rs # src/config/manifest.rs # src/config/rt.rs # src/pipelines/copydir.rs # src/pipelines/rust_app.rs # src/serve.rs # src/watch.rs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love the work, @hamza1311. Thank you for doing this. Just a few small nitpicks here and there. Otherwise, looks good to me 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much @hamza1311
@hamza1311 I'm in the process of adding an update merge to your branch here, and I'll make a few other changes (instead of going through the review process) |
The primary motivation behind these changes is to address the issue with the `nipper` crate exposing primarily !Send + !Sync types. The only pipeline which needs to deal with these types is now the HTML pipeline. All other pipelines receive a data-only representation of the original HTML.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hamza1311 ok, I've finished up a round of changes/updates on the code. Definitely take a look at the code & commit messages. Nothing too crazy, though one of the commits involved some refactoring just to enable the use of local_spawn
.
Overall, everything is ready to go except for the proxy code. It needs some work still. There are unwraps in the code, and I do not want to drop stacktraces on Trunk users. We've got a few options:
- I'm happy to go in and refactor that code. There are a few simple changes we can make to some of the warp handler functions to have them use a custom error type which wraps an
anyhow::Error
— which I've used quite successfully in a few of my other projects which use Warp. That will allow us to propagate errors normally instead of unwrapping/panicking. - Feel free to make those changes yourself, though I should have time to finish this up over the next few days as well. Just ping me if you start hacking on the changes. I'll do the same.
@thedodd, I removed the |
Ok, I should have the final changes for this PR finished up tonight. |
This pull request
async_std
totokio
As a result,
tide
withwarp
This should also help with #4 as there is support for web sockets.
There has been a change in proxy API.
proxy-rewrite
is replaced withproxy-path
. Now,proxy-path
denotes the route where trunk will listen.Fixes: #81, #95, #53
Checklist