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

Core isolation - part 4 #258

Merged
merged 19 commits into from
Jun 24, 2019
Merged

Conversation

prasannavl
Copy link
Contributor

@prasannavl prasannavl commented May 22, 2019

Description

  • Move Server, App to tide; make tide-core more minimal; tide-core no longer takes a few additional dependencies like http-service-hyper and features flags are now directly on tide.
  • Move internally shared items into tide-core::internal
  • Introduce constructors for Next, Selection and Cause.
  • Router is now abstracted into the router module in tide
  • Move EndpointResult from error module into endpoint module, as that seems more natural.
  • Reexport Body from http-service as it's commonly needed outside - and we already re-export Response. This lets users not to take an explicit dependency just for http-service::Body.
  • Refactor the error module to make things easier to follow

Others

  • Removed: err_fmt macro
  • Moved: cors example into tide examples since that seems to be norm with all examples going into main repo, while using doc based examples on the crate (for smaller middleware atleast)

Motivation and Context

#162 #252 #255

How Has This Been Tested?

All tests passed

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactor

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@prasannavl prasannavl changed the title Core isolation - part 4 [WIP] Core isolation - part 4 May 22, 2019
pub(crate) endpoint: &'a DynEndpoint<State>,
pub(crate) params: Params,
#[allow(missing_debug_implementations)]
pub struct Selection<'a, State> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might need to merge Context-building logic of tide::App into Router::route in order to remove this.

@prasannavl
Copy link
Contributor Author

prasannavl commented May 26, 2019

Wanted to do a little bit more with the routing extraction, but was quite busy this week, so decided to wrap this PR and get it into a merge-able state (if everyone's fine with the changes)! Hope it looks okay!

@prasannavl prasannavl changed the title [WIP] Core isolation - part 4 Core isolation - part 4 May 26, 2019
@prasannavl prasannavl requested review from Nemo157 and tirr-c May 26, 2019 00:22
[features]
default = ["hyper"]
hyper = ["http-service-hyper"]
serde_derive = "1.0.91"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor tidbit, but should have a newline at EOF

Copy link
Contributor Author

@prasannavl prasannavl May 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ha, will do when doing the rebase! 👍

@yoshuawuyts
Copy link
Member

Stabilizing Core

I thought when we were talking about trying to stabilize the core parts first, we would take away as many parts as possible. I ran the docs command, and there seem to be quite a few things left that I'm not sure should be included by default:

2019-05-27-162646_1920x1080

So my question is: what's the plan around only stabilizing the core parts of tide? I feel we talked about it in #162 (comment), but feel we might still not be aligned.


Router

I'm unsure about the benefits of moving out the router into its own crate. For example when looking at the docs Route isn't documented anywhere inside tide, whereas it ought to be a top-level export.

This could be easily fixed, but it feels like everything is quite tightly integrated, which makes me wonder about why we're moving this into their own crate in the first place. We probably wouldn't want to individually version / publish the router, and no crates other than Tide would likely want to depend on it.

Also as mentioned in this patch moving the routing into tide-router is only partially done in this patch because a lot of it is tightly integrated into Tide. I'm hesitant about this in-between state if we're unsure where we're going with this.

Perhaps it might be better to not move the router into its own crate until we're sure about the value it would deliver + we know where to draw the module boundaries.

@tirr-c
Copy link
Collaborator

tirr-c commented May 30, 2019

I agree that it's tough to extract router into another crate. Current design of the router is tightly integrated into Tide core; we need a different design if we're going to split this out of core.

@fairingrey
Copy link
Contributor

Just a status update, but #271 will need to be resolved before this is merged

@prasannavl
Copy link
Contributor Author

prasannavl commented Jun 5, 2019

Sorry, been a bit busy last week, couldn't wrap this up.

@yoshuawuyts - Following up with our discussion, the extras showing up like err_fmt is actually a bug. And the router bit - My intention is to move it back in the next commit, as discussed.

@fairingrey - While I'm in favor of a more flexible routing design, in the context of this PR, I think I align with @yoshuawuyts to move it back - Personally, I'd be surprised if the routing RFC comes to a speedy conclusion (Disclaimer: I haven't really had a chance to read it in detail yet), or a better question is if it even should be rushed into one - as if there's is a strong consensus to moved, I think it'd be good to take the time to come up with a solid impl plan in the RFC. What do you think?

@yoshuawuyts
Copy link
Member

@prasannavl I'm in favor of making the smallest amount of changes needed to move forward, but including the reversals you mentioned. I think we would benefit from doing another release sooner rather than later so folks get a chance to actually test out the changes (:

@fairingrey
Copy link
Contributor

That's a good point, I agree -- I'd say once we move the routing back in this PR (as it may take a while for the routing RFC to resolve) then we can just merge this and get people on this version of Tide.

Looking forward to it!

@prasannavl prasannavl requested review from Nemo157 and yoshuawuyts and removed request for Nemo157 and yoshuawuyts June 17, 2019 16:20

[dependencies]
futures-preview = "0.3.0-alpha.16"
http = "0.1"
http-service = "0.2.0"
http-service-hyper = { version = "0.2.0", optional = true }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[imo] I wanted an empty line below.

@yoshuawuyts yoshuawuyts merged commit 8f80e0c into http-rs:master Jun 24, 2019
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.

6 participants