Split tide into smaller crates #220

wafflespeanut · 2019-05-14T12:13:36Z

The following changes have been made (so far):

Extracted core types and traits from tide into tide-core
Removed unnecessary deps
Extracted cookie ContextExt and middleware into tide-cookies and feature-gated.

Description

This is a follow-up to #219 and so it's based on top of the commits introduced in the other PR.

Motivation and Context

Based on the discussion from #162

How Has This Been Tested?

It's a refactor which only affects the existing structure. It doesn't break anything.

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.

tide-core/Cargo.toml

tide-core/src/app.rs

tide/Cargo.toml

prasannavl

Thank you @wafflespeanut! This is good work.

I was previously waiting on some sign offs to get into the implementation details of this split. But now that we have, I think this is a good place for us to start asking the hard questions:

What exactly goes into tide-core.
What goes into tide.
What goes into the crates.

Relevant comments for some goal insights:

#162 (comment)
#162 (comment)

The key with tide-core, is that we want to keep this as minimal as possible and only exposes what the developers of tide extensions need (though I think for the moment, to keep things simple we can only focus on middleware authors atleast for the first phase).

With that in mind, (being extremely ideal for the first pass):

Middleware and Context are things that absolutely have to go into the core.
IntoResponse (or TryInto<Response> impls) is a bit tricky - Ideally this moves across to http-service itself since it provides no value to tide specifically and Response is a type owned by http-service which makes perfect sense since it's a http generic response and is best handled there itself so other crates which only want to use http-service can use them directly. So, what do we want to do about it during the split?
Error is the next bit. Here, (being idealistic again), I'd think this is candidate for http-service as well, since all it does is wrap Response<Body> and provide a http generic error. It makes sense this goes where-ever IntoResponse goes.
Endpoint and EndpointResult are the next question. Do middleware/extensions need them in way or is it just end users?
Next comes the question of Server, App - Is there any reason for them not to stay in just tide?
Stuff like cookies and form I think can go directly into their crates and be exposed from tide, or if there is no need for a crate - into tide directly.

@wafflespeanut - I know you were a bit keen to get this merged. But I think it's a good idea for this PR to be a bit more involved and answer atleast some of the above questions right. :) .. Would love to hear your thoughts from your experience on moving all these types around so far!

(Sorry if this suddenly ends up looking like a lot more than it seemed off the bat - we should be able to knock this off one by one quickly)

/cc @rustasync/maintainers

wafflespeanut · 2019-05-16T06:53:40Z

We should discuss this in the relevant RFC, as we've already arrived at a consensus for tide, tide-core and how middlewares should be implemented. The goal of this PR is to restructure the existing codebase without affecting its usage/functionality.

I agree that tide-core should be as minimal as possible, but I don't think we should achieve that in a single PR, as it's getting complicated as we address deeper issues.

I also believe that chopping an epic issue into smaller tasks and landing small units of work is much more effective compared to blocking all tasks related to an RFC on one individual. If we land this now, then relatively smaller tasks like say, moving some type to http-service could be mentored work for newcomers. It doesn't necessarily have to be done by me (I will do it, just making a point).

One other (purely selfish) reason to land this is that restructuring PRs are fragile. It's already broken twice and it's analog work to go through the newly landed commits to ensure that a rebase was done properly. I'm pretty sure if we boot up another discussion now, then by the time we arrive at a consensus, it's gonna break a few more times again.

In addition to being minimal, tide-core should also do what its name implies - have the core types and traits. With that, I'll address the concerns:

It's sensible to move Response-convertible traits into http-service
While it makes sense to have an error type in http-service, I think tide should have its own error.
Endpoint is a core trait, so it should be in tide-core, whereas EndpointResult is a convenience type, so it can probably move to tide? (I don't wanna conclude anything based on that just yet).
We have a bunch of private entities (say, initializing Context, Next, etc.) - App and Server make use of such entities. That's why I moved them to core. Having App and Server in tide implies that we have to expose those entities publicly.
I feel tide-cookies as a separate crate is a nice segregation (same applies to future middlewares) as it isolates dependencies and aids semver.

prasannavl · 2019-05-16T08:51:30Z

@wafflespeanut - I'm in a bit of time crunch, but I'll address some of the non technical bits really quickly.

We should discuss this in the relevant RFC, as we've already arrived at a consensus for tide, tide-core and how middlewares should be implemented. The goal of this PR is to restructure the existing codebase without affecting its usage/functionality.

I agree that tide-core should be as minimal as possible, but I don't think we should achieve that in a single PR, as it's getting complicated as we address deeper issues.

I get the feeling you may have misunderstood.

The key is to get the insight so that we can be cognisant of where this split will leave us, and as such if there's a compelling reason to change specific bits about this PR, to do so before merging - as the consensus for tide, tide-core, and middleware are quite tied to this PR as well.
Things like IntoResponse, Error are definitely RFC materials - My question here is how do we go about handling them in the context of this split - "Leave it in tide_core" could be valid answer too, as long as the reviewers agree.

One other (purely selfish) reason to land this is that restructuring PRs are fragile. It's already broken twice and it's analog work to go through the newly landed commits to ensure that a rebase was done properly. I'm pretty sure if we boot up another discussion now, then by the time we arrive at a consensus, it's gonna break a few more times again.

First off, I'm sorry that you had ended up with this feeling in the first place. Early stage restructuring PRs are usually tricky to handle, and couple that with early stage open source project, and this becomes quite challenging to come up with a comprehensive design plan before hand to avoid this.
I'd ask that you not worry about this too much. It's neither fair nor needed for you alone to take the burden of keeping up with the changes. We're here to help (and while I can't speak for others, if you'd like, I'd be happy to help rebase and keep up with the changes if that helps us stay on the right track). But I personally think this by itself a wrong reason to merge any PRs.
That being said, this is your PR and the decision of whether you'd be okay with someone else helping you rebase your PR is upto you :).

The ultimate goal here is to gather the reviews, and then make sure the PR is something that can be structured into the limitations of your interest and time, while not compromising on the quality -- if it isn't already.

PS. Also, if you could, I think it'd be great for you bring this PR up, as the rustasync weekly meeting fortunately is in a couple of hours! (I'd guess it'll help speed this up).

wafflespeanut · 2019-05-16T09:51:35Z

I'm sorry if I'm misunderstanding something.

If there's a compelling reason to make any changes in this PR, I'd be happy to do that! Re-iterating my argument, this PR aims to address one thing without introducing any breaking changes and if we all agree on what it does and how we can work on the changes introduced by it incrementally, then we can move on to the next task, instead of waiting on the PR to address everything related to the split as it's inefficient IMO.

In short, I'm only worried about making this PR address tasks that could otherwise be done incrementally (in subsequent PRs, which is also easier once we have the initial structure). Rebasing and keeping up with the changes isn't tricky (it's just boring analog work), I'm not worried about it and I'm definitely not suggesting that as a reason to merge this PR. :)

yoshuawuyts

Few nits, overall looks good!

README.md

tide/src/lib.rs

wafflespeanut · 2019-05-16T19:11:11Z

Fixed and rebased!

tide/src/error.rs

fairingrey · 2019-05-17T05:37:06Z

Looks good to me, thanks for your work!

wafflespeanut · 2019-05-17T05:53:18Z

Thank you all for reviewing!

prasannavl · 2019-05-17T08:59:50Z

I'm a bit busy this weekend, and I've not been able to make time to think through more clearly some of potential directions of what I expressed above just yet.

If everyone else is cool with the PR, I'll jump back into the extraction bits post merge :)

Only thing I'd like to strongly insist and make sure is that tide-core remains a private dependency for now until we can answer most of those questions, and do the refactoring needed, since @yoshuawuyts is planning on cutting a new release soon.

Nemo157 · 2019-05-17T09:10:17Z

tide-core appears to be as much of a private dependency as it can be at this stage, it's not mentioned anywhere I can find in the documentation of tide, and I doubt users will just start depending on it without understanding what it is for.

Nemo157 reviewed May 14, 2019

View reviewed changes

tide-core/Cargo.toml Outdated Show resolved Hide resolved

tide-core/src/app.rs Outdated Show resolved Hide resolved

prasannavl mentioned this pull request May 15, 2019

RFC: isolating a stable core #162

Open

wafflespeanut marked this pull request as ready for review May 15, 2019 07:17

Nemo157 reviewed May 15, 2019

View reviewed changes

tide/Cargo.toml Outdated Show resolved Hide resolved

prasannavl mentioned this pull request May 15, 2019

Reorganize examples and make runnable #227

Closed

9 tasks

Nemo157 mentioned this pull request May 15, 2019

Update travis configuration #228

Merged

8 tasks

prasannavl added this to ✔️Todo in Async ecosystem sprint #2 (2019-05-16 — 2019-06-20) via automation May 16, 2019

prasannavl moved this from ✔️Todo to ⏳In Progress in Async ecosystem sprint #2 (2019-05-16 — 2019-06-20) May 16, 2019

prasannavl reviewed May 16, 2019

View reviewed changes

yoshuawuyts requested changes May 16, 2019

View reviewed changes

README.md Outdated Show resolved Hide resolved

tide/src/lib.rs Outdated Show resolved Hide resolved

tide/src/lib.rs Show resolved Hide resolved

prasannavl mentioned this pull request May 16, 2019

add tide-log #222

Merged

8 tasks

fairingrey suggested changes May 16, 2019

View reviewed changes

tide/src/error.rs Outdated Show resolved Hide resolved

wafflespeanut added 3 commits May 17, 2019 10:32

Move core traits and types to tide-core

b6ff2b2

Remove unncessary dependencies and use relative paths in README

1185dd1

Move cookies middleware to tide-cookies

7cc05ab

fairingrey approved these changes May 17, 2019

View reviewed changes

fairingrey requested a review from yoshuawuyts May 17, 2019 06:16

prasannavl mentioned this pull request May 17, 2019

Move enforcement of crate level forbid and deny from compiler to CI #232

Closed

yoshuawuyts approved these changes May 19, 2019

View reviewed changes

yoshuawuyts merged commit 77b3a1c into http-rs:master May 19, 2019

Async ecosystem sprint #2 (2019-05-16 — 2019-06-20) automation moved this from ⏳In Progress to 🚀 Done May 19, 2019

yoshuawuyts mentioned this pull request May 19, 2019

Add error::ResultDynErrExt #216

Merged

8 tasks

wafflespeanut deleted the code-split branch May 19, 2019 18:06

yoshuawuyts removed this from 🚀 Done in Async ecosystem sprint #2 (2019-05-16 — 2019-06-20) Oct 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split tide into smaller crates #220

Split tide into smaller crates #220

wafflespeanut commented May 14, 2019 •

edited

Loading

prasannavl left a comment •

edited

Loading

wafflespeanut commented May 16, 2019

prasannavl commented May 16, 2019

wafflespeanut commented May 16, 2019 •

edited

Loading

yoshuawuyts left a comment

wafflespeanut commented May 16, 2019

fairingrey commented May 17, 2019

wafflespeanut commented May 17, 2019

prasannavl commented May 17, 2019 •

edited

Loading

Nemo157 commented May 17, 2019

Split tide into smaller crates #220

Split tide into smaller crates #220

Conversation

wafflespeanut commented May 14, 2019 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

prasannavl left a comment • edited Loading

Choose a reason for hiding this comment

wafflespeanut commented May 16, 2019

prasannavl commented May 16, 2019

wafflespeanut commented May 16, 2019 • edited Loading

yoshuawuyts left a comment

Choose a reason for hiding this comment

wafflespeanut commented May 16, 2019

fairingrey commented May 17, 2019

wafflespeanut commented May 17, 2019

prasannavl commented May 17, 2019 • edited Loading

Nemo157 commented May 17, 2019

wafflespeanut commented May 14, 2019 •

edited

Loading

prasannavl left a comment •

edited

Loading

wafflespeanut commented May 16, 2019 •

edited

Loading

prasannavl commented May 17, 2019 •

edited

Loading