carllerche / tower-web

A fast, boilerplate free, web framework for Rust
MIT License
980 stars 51 forks source link

Edition 2018 updates #211

Closed elpiel closed 5 years ago

elpiel commented 5 years ago

Opened questions:

TODOs:

Maybe Resolves #37

elpiel commented 5 years ago

Build is not triggered or something + the opened question still remains, but I guess this can also be reviewed already

elpiel commented 5 years ago

@carllerche is it possible to continue with this? I would like to continue with making async/await working again with latest nightly and the .await syntax

lnicola commented 5 years ago

@elpiel sorry for not looking into this earlier, it kinda' slipped off my mind. I'm not sure if you're still using tower-web. I guess everyone is now waiting for await to hit stable, and @carllerche is busy updating tokio and other parts of the ecosystem.

elpiel commented 5 years ago

@lnicola we are thinking of migrating, because of various of reasons, but I will be happy to help more to migrate to .await and I will definitely be happy if this gets merged.

lnicola commented 5 years ago

Yeah, I was using tower-web myself in a small app, but I've migrated back to hyper, since I didn't get that much benefit from it.

Can you try pushing a small commit (or rebase) to kick Travis?

elpiel commented 5 years ago

Nightly is failing, but that's expected.

lnicola commented 5 years ago

Looks like all of them are failing?

error[E0658]: The attribute serde is currently unknown to the compiler and may have meaning added to it in the future

And a double semicolon.

elpiel commented 5 years ago

Yeah, by bad, actually it needs use serde::Serialize, but I now get another error trying to build the test_suits:

   --> src/lib.rs:494:1
    |
494 | / proc_macro_item_decl! {
495 | |     #[doc(hidden)]
496 | |     derive_resource! => derive_resource_impl
497 | | }
    | |_^
    |
    = help: message: assertion failed: `(left == right)`
              left: `Some("#[allow(unused,")`,
             right: `Some("#[allow(unused")`
lnicola commented 5 years ago

I don't see that with

diff --git i/src/middleware/cors/config.rs w/src/middleware/cors/config.rs
index ea9d3dc..2b09946 100644
--- i/src/middleware/cors/config.rs
+++ w/src/middleware/cors/config.rs
@@ -188,7 +188,7 @@ impl Config {
     }

     fn common_headers(&self, origin: HeaderValue) -> HeaderMap {
-        let mut headers = self.basic_headers();;
+        let mut headers = self.basic_headers();

         if self.allow_credentials {
             headers.insert(header::ACCESS_CONTROL_ALLOW_ORIGIN, origin);
diff --git i/tower-web-macros/src/derive/response.rs w/tower-web-macros/src/derive/response.rs
index 079b981..9c8504c 100644
--- i/tower-web-macros/src/derive/response.rs
+++ w/tower-web-macros/src/derive/response.rs
@@ -380,7 +380,7 @@ impl Response {
         let shadow_data = &self.shadow_ty;

         quote! {
-            #[derive(Serialize)]
+            #[derive(serde::Serialize)]
             #[serde(remote = #ty)]
             #shadow_data
         }
elpiel commented 5 years ago

Yes, this is true. I was trying to fix the build another way.

lnicola commented 5 years ago

I think there are still some unused import warnings (for serde::Serialize). Might be a good idea to fix those, if it's easy.

I actually wasn't sure that #[derive(serde::Serialize)] works, but it might be a better approach than an use statement. Although should it be __tw::codegen::serde::Serialize instead?

elpiel commented 5 years ago

I think now that it is importing serde rather than having a use statement, it will trigger some unused imports indeed.

Although should it be __tw::codegen::serde::Serialize instead? I have no idea, I've just started with macros and cannot give any answer.

Also nightly fails for a weird reason: https://travis-ci.org/carllerche/tower-web/jobs/581655083#L465

lnicola commented 5 years ago

Can you also bump the compiler version to 1.34 in .travis.yml?

lnicola commented 5 years ago

The CI is happy now, and is @carllerche is probably missing, so I think I'll merge this. Thanks!