http-rs / tide

Fast and friendly HTTP server framework for async Rust
https://docs.rs/tide
Apache License 2.0
5.06k stars 322 forks source link

RFC: isolating a stable core #162

Open aturon opened 5 years ago

aturon commented 5 years ago

Tide is designed to be a highly modular, extensible framework, and #156 pushes us further in that direction.

There are a set of core concepts and interfaces:

Beyond that, we have a number of implementations and extensions:

Tide's current design allow for a strong decoupling between the core interfaces and these various extensions, much like Flask. This raises some questions about how to approach development, as well as the UX for Tide.

Proposal: follow the Flask model (for now)

Since Tide is explicitly intended to foster re-usable, modular components that can work with other frameworks, it makes a lot of sense to take full advantage of the decoupling above. That would follow in Flask's footsteps, where there's an extensive ecosystem of independent extensions to the tiny core framework.

I propose that we:

Eventually, as we gain more experience, I imagine that some of these extensions will become de facto standard, and we might want to consider moving them into core. Likewise, I expect we'll want some "bundling" crates that set up a middleware stack for you, to make it very easy to get going with Tide. But these things can come later.

This approach would make it feasible to get the Tide core to 1.0 status relatively quickly, with the expectation that the APIs would remain stable for a while (let's say six months at least). Meanwhile, the various middlware crates etc can be versioned independently, freeing them to make breaking changes without any coordination.

Open questions

One open question: if we do take this direction, do we want separate repos or just separate crates? I worry a bit that if we get too aggressive about separating repos right now, it will be hard to follow what's happening. So I personally lean toward at least starting by having the other crates live within the tide repo, with a shared issue tracker, until we start hitting scaling problems.

wdyt?

aturon commented 5 years ago

Related to #73 and https://github.com/rustasync/tide/pull/159 as well, which both touch on "middlware stacks" (and how to provide a default one).

aturon commented 5 years ago

An important question here: as the dust settles from #156, can we find ways to make it easier to write middleware that works across multiple frameworks? E.g., Actix has its own notion of middleware that's not too far off from Tide's. But it's not obvious that the differences can be easily bridged. Maybe it's possible to define a "least common denominator" interface that offers generic wrappers for various frameworks.

rolftimmermans commented 5 years ago

Having a universal middleware concept in the Rust web-ecosystem is something I look forward to. In the Ruby community, for example, every framework builds on Rack middleware. I see this as the sole factor that allows for (some) code reuse across frameworks.

My questions would be:

Edit (from @aturon): I've created a dedicated issue for further discussion along these lines.

yoshuawuyts commented 5 years ago

if we do take this direction, do we want separate repos or just separate crates?

I think we can probably follow in gloo's footsteps here, and create a whole lot of subcrates + a skeleton to RFC a design for a crate. This would allow us to build out a fair share of middleware without incurring too much of the maintenance overhead.

Also in particular their approaches to versioning and release scope might be interesting to read up on.

prasannavl commented 5 years ago

Glad to see this direction!

Couple of quick thoughts:

bIgBV commented 5 years ago

@prasannavl re: logger, I think following the footsteps of actix would be a good idea. Simply use the macros from the log crate and users of the framework can use a crate which implements the Logger interface. This could be something like env_logger, slog or even tokio-trace. I think that this is the most flexible approach.

prasannavl commented 5 years ago

@bIgBV - That's exactly what the RequestLogger in surf does. Plus a few other goodies like timing, and more planned like custom formatter, all on top of whatever logger is being set up by the log crate by the application.

aturon commented 5 years ago

@rolftimmermans great comment -- I liked it so much I've moved it to its own issue, as I think cross-framework middleware deserves its own focused discussion.

(That said, as folks have noted in this thread, there's some potential impact on where middleware lives and how it's named etc, depending on whether it's Tide-specific)

mmrath commented 5 years ago

While there are advantages of keeping core small and then building app with pluggable ecosystem crates, I am more in favour of a meta crate that pulls together necessary bits to get me started. The basic things should all be provided by the crate. One approach to extract core apis to a crate(which is currently tide) named tide-api and then other crates depend of tide-api. tide can be the meta crates with enough batteries to get started without pulling 10 others crates directly. It is a bit of overhead if I have to choose crates for very basic things.

prasannavl commented 5 years ago

I'd rather prefer it handled outside of tide for the time being, as the tide story first becomes production ready and has a little bit of time to stabilise. This will put tide in a much better place on how best to do this. I've been examining how to get this up and running in surf myself.

I've been thinking of helper patterns similar to surf::app::new or surf::app::with_common, or if it should be called AppBuilder with a builder pattern -> both of which basically return a tide::App with batteries (common middleware, defaults, etc). Now, while it's just me with surf and have no concrete plans on it's future, I think in general it's a good idea for tide to encourage being experimented this way to gather a little more on it's usage patterns before rushing into something like tide-api.

secretfader commented 5 years ago

Discussions after today's meeting seemed to center around the need to separate Tide core from contributed extensions that, while they may be common, aren't needed for every application that depends on Tide.

Based on an idea from @bIgBV, I propose the following:

  1. All non-essential extensions live in a tide-contrib repo, created under this organization, which functions as a multi-crate workspace.
  2. Common macros that are needed by extensions in that repo can be placed in a shared crate. (If we need to bundle multiple crates into commonly used bundles, that's an option, too.)

175 is a solid example of a feature that essentially all Tide apps will need, while session handling is a concern that's slightly out of scope for core, in my opinion.

aturon commented 5 years ago

We had some extensive discussion on Discord, which led to the following.

Goals

Proposal

(from @yoshuawuyts)

yoshuawuyts commented 5 years ago

@rustasync/maintainers would anyone be interested in picking up this issue for this sprint? It seems like this would likely make the biggest structural improvement to our process, and free us up to do more experiments, and iterate faster :sparkles:

wafflespeanut commented 5 years ago

I can pick this up!

secretfader commented 5 years ago

You're welcome to it, @wafflespeanut. I'll be on the lookout for a PR. Thanks for helping out!

prasannavl commented 5 years ago

@wafflespeanut - Awesome! Splitting this up into smaller units to make this more manageable into smaller PRs with the current middlewares

Isolating Core - Stage 1

Precursor

(Non breaking, foundation bits for the entire organisational structure):


Core Isolation

(Initial run, we could even do most of the isolation in a non-breaking way - hopefully even everything, but fingers crossed and assume breaking for now)


Before next release

Middlewares

Others

secretfader commented 5 years ago

We can bikeshed on crate names later. I don't think that's the most critical portion of this discussion. Ideally, the PR addressing this RFC should initially focus on the following:

Splitting Tide core into smaller crates:

Before we move any new code into this repo (from Surf or elsewhere). It makes sense to document the expected outcome. Once the split between Tide core and middleware is complete, I expect the source tree will have two primary folders in the root: tide/, which will contain the core framework implementation, and contrib/, which will contain middlewares and context extensions that aren't needed by every application, each in it's own crate (but organized in such a way that we can roll-up common dependencies into convenience crates).

I'd also like to see dependencies for example code moved out of Tide's dependency list. However, I'm aware that some dependencies are shared between examples (serde, for one), which may complicate this suggestion.

Which extensions and middleware become their own crates?

I think @prasannavl's comment above is a good place to start. Cookies can be split into tide-cookies, and Tide's existing logging functionality can become tide-slog. Tide's form extraction should probably be moved to tide-form. Past that point, I'm not totally sure if the middleware that adds default headers is worth extracting yet. Most HTTP applications will need to manipulate headers, which is a fairly strong case for keeping it in core (and should probably be the standard that decides where all components reside).

The above text encapsulates minimal changes that I think would satisfy the RFC. We can decide on the naming conventions of new crates once the bulk of this work is completed.

prasannavl commented 5 years ago

@secretfader - I think we've already discussed this, and there's no special contrib as such for now. Just lots to tide- middlewares for now. (https://github.com/rustasync/tide/issues/162#issuecomment-484631906)

We can move whatever needs to be in the core later. For now, I think we just split off and hit the ground running marking this as a ground zero of sorts, with nothing in the core at the moment. Then, we can decide what moves to core in whatever evolved form after it has had time to stabilise.

Also, I don't really see the problem with serde, however if it does indeed get into a problem splitting it into an example, then I'd label that a bug -- that needs to be fixed.

Making the PR that does 1 now. Hopefully @wafflespeanut can chime in on what he'd like to pick up form there! :)

secretfader commented 5 years ago

@prasannavl I think you misunderstood my comment. I'm not suggesting we create a crate for commonly used middlewares at this stage. However, think it would be helpful to place any resulting crates into their own folder, contrib/, rather than tossing all extracted code into the repo root.

prasannavl commented 5 years ago

Tokio, futures all of them follow this model of doing it in the root currently. Besides, everything other than tide itself is going to be a folder with middleware, so I don't see the point of the indirection at this stage. However, if you feel strongly about the organisation structure, perhaps we should arrive at a consensus there.

(Note: This could totally be a different scenario when tide's mature and have so many that tide-contrib [ideally a repo] is needed for community extensions not directly supported by the tide maintainers)

prasannavl commented 5 years ago

@yoshuawuyts - I can co-ordinate this with others interested. Have done some of the foundation bits as well. I know you're busy on runtime this sprint, but would be great if you could just quickly run through the game plan here to make sure it all aligns well: https://github.com/rustasync/tide/issues/162#issuecomment-491825007

yoshuawuyts commented 5 years ago

@prasannavl

  • tide-core - Needed so that the other tide- can depend on this, and we can pull everything together into tide, or we end up with either having the whole thing in core, or have it separate with no transitive path where crates can graduate into tide in a non-breaking way (Since that will end in cyclic dependencies) #220
    • tide - Tide core + graduating dependencies.

I'm thinking the distinction of tide-core and tide isn't that important. Something we've tried out in https://github.com/rustasync/runtime/pull/22 is the addition of conditional dependencies based on targets.

I could imagine there would be a way to setup feature flags for Tide that by default you get what you need. And then there's a feature that when enabled removes all dependencies, and you can configure everything yourself.

I suspect that adding in this mechanism would be an extra 100 or so lines, but would allow us to streamline things reasonably (:


Separately from that runtime's dir structure is pretty nice; we use src for the runtime crate, so we never have to traverse through the nested runtime paths: runtime/runtime/src/lib.rs becomes runtime/src/lib.rs. But don't feel too strongly about this (:

Nemo157 commented 5 years ago

One of the primary usecases of a separate -core crate is to allow semver breaking updates of the non--core crates if those are primarily used as private dependencies of libraries/applications. As long as all public API of downstream crates use only types from the -core crate they can interoperate even if they're using different versions of utilities internally. (This is why futures is split the way it is).

If you expect to never have a semver breaking update of anything that gets graduated into tide then the split is unnecessary, and it's probably possible to do a backwards compatible split in the future if needed as long as all non-core parts are feature-gated from the beginning (similar to how log has provided compatibility between 0.3 and 0.4).

prasannavl commented 5 years ago

I'm thinking the distinction of tide-core and tide isn't that important. Something we've tried out in rustasync/runtime#22 is the addition of conditional dependencies based on targets.

@yoshuawuyts I think @Nemo157 put across what I had in mind beautifully with the future backed experience. It'd be simpler for the middleware crates to iterate and grow easily without tying into the whole batteries included tide. Target based dependencies fit naturally for the runtime world - but I think the futures approach is a much better fit for tide.

That being said, if we don't have consensus - one way to delay this is to keep tide-core a private dependency for now that's only taken on by tide- in this repo. For the rest of the world, it's exactly as tide functions now.

Also, while this is easily understood, will just iterate for the sake of completion - tide-core ideally gets only absolute essentials that are cherry-picked from tide proper. Since we have essential middlewares in the same repo as well, we should be able to get a use-case driven guidance on that instead of exposing things like App, Server etc from the get go.


Separately from that runtime's dir structure is pretty nice; we use src for the runtime crate, so we never have to traverse through the nested runtime paths: runtime/runtime/src/lib.rs becomes runtime/src/lib.rs. But don't feel too strongly about this (:

Yes. I liked the structure when I saw in it gloo as well. But decided to adopt futures and tokio style dir for the moment - But I don't really have any preference regarding the runtime dir. Should easy enough to switch out as well should you prefer to have it that way.

yoshuawuyts commented 5 years ago

If you expect to never have a semver breaking update of anything that gets graduated into tide then the split is unnecessary

@Nemo157 yeah, that was the exact idea I had in mind. Things only graduate once we're ready to commit them feeling like the external API is ready to be tied to Tide's stability model.

But given that they'd be used internally to Tide a lot of the times, that constraint probably feels even more flexible.


@prasannavl Up until now I was under the impression we'd achieved consensus on having a single top-level tide, without the need for -core crate when we consolidated https://github.com/rustasync/tide/issues/162#issuecomment-484631906. Perhaps there was a miscommunication?

Tide vs Tide-Core

I feel we've seen a lot of experimentation around core vs ext libs, (e.g. futures-core, tokio-core), and generally it seems people mostly care about not having to worry about what to use. I suspect that a distinction between tide-core and tide doesn't buy us any extra semver mobility, because almost everyone would opt to use tide directly.

Similarly we can probably learn from syn, where a lot of the features are hidden behind the "full" flag, which is also what most people want to be using. So instead of making the default experience opt-in, we make the stripped down experience the deviating choice.

# full
[dependencies]
tide = "1.3.2"

# minimal
[dependencies]
tide = { version = "1.3.2", features = [ "bare" ] }

Graduating dependencies

But in order to encourage productivity, we probably want to provide people with ways of trying out new additions, after we feel that things might be good, but before we commit to stabilizing.

# stable
[dependencies]
tide = "1.3.2"

# experimental
[dependencies]
tide = { version = "1.3.2", features = [ "cookies-experiment" ] }

I'm not 100% convinced how important this last bit is, but it might be something to consider if we want to enable some nicer workflows. It's not something we have to decide now, but can revisit as we start thinking of graduating dependencies to stable.

spacekookie commented 5 years ago

Just wanna chime in and say that I very much agree with @yoshuawuyts idea of having the default experience be as inclusive as possible.

What always annoyed me so far with a lot of web service development in Rust is the shear number of imports and external crates required to explicitly mention.

Having tide as one dependency with no additional features doing what you probably want will remove a lot of barriers.

Similarly, I feel like we should be sparse with feature flags. Having a generic unstable flag is something I've seen on some crates and should generally give people enough space to play around with APIs before committing to them, without again making a user include 12 different feature flags.

prasannavl commented 5 years ago

Before anything, let me clarify this: Everything that's being discussed above, from a user perspective is a single tide dependency only. User of tide just use tide. This is of relevance only to tide extension, development and middleware authors.

@prasannavl Up until now I was under the impression we'd achieved consensus on having a single top-level tide, without the need for -core crate when we consolidated #162 (comment). Perhaps there was a miscommunication?

Yes. This is true. It appears I'm the one who didn't get the nuances of the details correctly. Sorry about that if it was my bad - I naturally ended up with the split, because that's the only way to really have tide- repos for the middleware currently in the tide repo (which again to my understanding was the decision taken). So, I was under the impression that was only a question of what was exposed publicly.

I'll put things in context here:

Tide only

tide-core + tide

From a user's perspective this is still just one dependency tide.

tide-core is only for middleware authors, tide developers eitherway.

Nemo157 commented 5 years ago

I suspect that a distinction between tide-core and tide doesn't buy us any extra semver mobility, because almost everyone would opt to use tide directly.

You can still directly refer to tide and get semver mobility, if you have crate foo depending on tide v0.1 and crate bar depending on tide v0.2 they will still have API compatibility as long as the public types they mention are those from tide-core v0.1 which both versions of tide depend on. If they mentioned specific middleware in their public APIs then there will be semver compatibility issues if those middleware have a breaking change, but it seems like most APIs should be built on the types from tide-core (even if they're referred to from a path starting at tide).

Nemo157 commented 5 years ago
# minimal
[dependencies]
tide = { version = "1.3.2", features = [ "bare" ] }

this should be

# minimal
[dependencies]
tide = { version = "1.3.2", default-features = false }

or you're going to have a lot of issues related to bad feature interactions. If you want all features on by default you just need to specify all features in the default feature (e.g. that's how I just setup async-compression so that users could choose which compression schemes to support while defaulting to having everything enabled).

prasannavl commented 5 years ago

@Nemo157 - Yes, this is the only way to allow for things to be in separate crates wihout tide-core as far as I know. I see this as being far more inconvenient rather than just using tide-core from a middleware perspective. This exactly what I mentioned while I said,

We can't have middleware and tide managed independently while being exposed from the tide crate, as that involves a cyclic dependency (unless we resort to more feature based dependency shenanigans here which could not be the most ergonomic)

Very simply put features flags are easy to miss, pain to manage, rather than just a simple tide-core and not worry, from a middleware author's perspective.