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

introduce better app configurability #159

Closed
wants to merge 1 commit into from

Conversation

prasannavl
Copy link
Contributor

@prasannavl prasannavl commented Apr 3, 2019

Currently, App::new() automatically sets up logging middleware. When this is not desirable there is not way to remove it.

While configurability of tide in general is an ongoing process, and will change as we go, I expect some form default middleware and other options to be pre-configured, logging being a prime example. However there should also be a way out for people who'd prefer starting from scratch.

So, as the configuration story improves, I think a nice to way to handle this throughout (either for just the current evolutionary phases of tide, or through to more mature phases if it desired), would be to introduce to a method App::empty() which is basically new with bare bones. And new as usual, provides sensible defaults.

@yoshuawuyts
Copy link
Member

@prasannavl I think this direction is pretty good! An alternative structure is to create a builder where each feature can be enabled manually (tho saying that out loud makes me realize that's not necessary in contradiction to the API you're proposing). This seems like a reasonable enough addition.

However we should probably block merging this until #156 is done to see how it fits into the larger picture.

@prasannavl
Copy link
Contributor Author

Oh, just looked into the diff of #156. You're right, doesn't make much sense to merge at this point.

PS. I did think about the builder, but just felt it was overkill at this stage. Especially considering Configuration already had builder and didn't like the idea of having builder after builder as opposed to the nice API it currently offered. That said, looks like #156 should make all this better and bring more clarity.

@mehcode
Copy link
Contributor

mehcode commented Apr 6, 2019

Can I suggest ::new() is "empty" and ::default() is what ::new() is now? Seems more intuitive to me.

@prasannavl
Copy link
Contributor Author

@mehcode - That was my first line of thought too. Unfortunately though, since default can't take an argument which is needed for AppData, default wouldn't work well with the current setup without tweaking the API in a breaking way, so went this direction for now. Meanwhile, let's wait out for #156 :)

@prasannavl
Copy link
Contributor Author

prasannavl commented Apr 6, 2019

Just giving a little bit of thought to #156, since there App::new just returns an empty one. I see multiple ways to do this.

  • Create another App constructor - Say, App::default or App::basic or App::common (name bikeshedding needed), that could do this. (Or App::with_default to be in line with rust constructor conventions)
let mut app = App:with_default(Data); // Note this is not the `Default` trait.
app.serve(addr)
  • Instead of handling this on the App, move this to middleware module, that borrows the app, and does it, with this being an additional step: Say, middleware::use_defaults(&App)
let mut app = App::new(Data);
tide::middleware::use_defaults(&mut app);
app.serve(addr);
  • Continue the pattern of app being a builder, and add App::use_defaults (bikeshed names) for a builder.
let mut app = App::new(Data);
app.use_defaults();
app.serve(addr);

(or) even

let mut app = App::new(Data).defaults();
app.serve(addr);

@aturon
Copy link
Collaborator

aturon commented Apr 10, 2019

Yep, with #156 we'll need to think through this question again. It's also tied to how we want to divide up the "core" framework versus middleware and other extensions -- I'll open up a distinct issue on that for further discussion.

@aturon
Copy link
Collaborator

aturon commented Apr 10, 2019

Issue up: #162

@yoshuawuyts
Copy link
Member

@prasannavl can this be closed now that #162 is up?

@prasannavl
Copy link
Contributor Author

Yup! Was thinking of doing a cleanup run myself just now. Closing.

@prasannavl prasannavl closed this Apr 17, 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.

4 participants