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

Options Method #104

Open
wants to merge 4 commits into
base: master
from

Conversation

@tzilist
Copy link
Contributor

tzilist commented Dec 4, 2018

Description

Tide will now automatically implement the OPTIONS method by checking which methods are available for a specific path. Currently, the implementation fails to compile with this error message and I am stuck trying to figure out what the issue is :/

error[E0277]: the trait bound `[closure@src/router.rs:214:26: 222:14 option_header:_]: endpoint::Endpoint<Data, _>` is not satisfied
   --> src/router.rs:214:18
    |
214 |             self.options(async || {
    |                  ^^^^^^^ the trait `endpoint::Endpoint<Data, _>` is not implemented for `[closure@src/router.rs:214:26: 222:14 option_header:_]`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0277`.
error: Could not compile `tide`.

To learn more, run the command again with --verbose.

Any help would be greatly appreciated!

Motivation and Context

closes #51

How Has This Been Tested?

Not yet, implementation currently does not compile

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)

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.
@bIgBV

This comment has been minimized.

Copy link
Contributor

bIgBV commented Dec 4, 2018

So I dug around a bit and I think it has to do with the fact that the endpoint function is being constructed in tide vs tide being linked as a dependency to a different binary. I tried simplifying the logic and removed the recursive call and reduced the code to the following:

    let resource = resource.as_mut().unwrap();

    if resource.endpoints.contains_key(&method) {
        panic!("A {} endpoint already exists for this path", method)
    }

    if !resource.endpoints.contains_key(&http::Method::OPTIONS) {
        resource.method_options.push(method.as_str().to_string());
        let option_header = resource.method_options.join(", ");
        let callback = async || {
            http::Response::builder()
                .status(http::status::StatusCode::OK)
                .header("Content-Type", "text/plain")
                .body(Body::empty())
                .unwrap()
        };
        resource
            .endpoints
            .insert(http::Method::OPTIONS, BoxedEndpoint::new(callback));
    }
    resource.endpoints.insert(method, BoxedEndpoint::new(ep));
}

And now I'm getting a similar error, but with more information:

error[E0277]: the trait bound `Data: std::clone::Clone` is not satisfied
   --> src/router.rs:222:48
    |
222 |                 .insert(http::Method::OPTIONS, BoxedEndpoint::new(callback));
    |                                                ^^^^^^^^^^^^^^^^^^ the trait `std::clone::Clone` is not implemented for `Data`
    |
    = help: consider adding a `where Data: std::clone::Clone` bound
    = note: required because of the requirements on the impl of `endpoint::Endpoint<Data, (endpoint::Ty<impl core::future::future::Future>,)>` for `[closure@src/router.rs:213:28: 219:14]`

The same error is repeated for Send and Sync as well. Which I find weird, as this closure is not any different than the one found in examples/default_handler.rs. So I think it has to do with the way the Endpoint trait is implemented. Might be wrong, but I'm not sure how to proceed from here.

Maybe @aturon might have some more insight into this.

src/router.rs Outdated Show resolved Hide resolved
src/router.rs Outdated Show resolved Hide resolved
@yoshuawuyts yoshuawuyts added the feature label Jan 10, 2019
@yoshuawuyts

This comment has been minimized.

Copy link
Member

yoshuawuyts commented Jan 10, 2019

ping @tzilist do you have any status updates on this PR? Do reckon you have the bandwidth to make the final changes?

@tzilist

This comment has been minimized.

Copy link
Contributor Author

tzilist commented Jan 10, 2019

@yoshuawuyts Apologies, work picked up over the holidays and I probably wont have a ton of time to work on this for a while :( if someone else wants to pick it up that'd be great! I hope to come back and contribute some more in the next month or so!

@yoshuawuyts

This comment has been minimized.

Copy link
Member

yoshuawuyts commented Jan 11, 2019

@tzilist no worries; I appreciate the quick reply!

I might pick up the suggested changes, and try to get this landed. Would be really cool to have this patch in Tide I think!

@yoshuawuyts yoshuawuyts changed the title [WIP] Options Method Options Method Jan 11, 2019
@yoshuawuyts yoshuawuyts force-pushed the tzilist:options-method branch from 6318f00 to da87f0f Jan 11, 2019
tzilist and others added 4 commits Dec 4, 2018
wip
Signed-off-by: Yoshua Wuyts <yoshuawuyts@gmail.com>
Signed-off-by: Yoshua Wuyts <yoshuawuyts@gmail.com>
Signed-off-by: Yoshua Wuyts <yoshuawuyts@gmail.com>
Signed-off-by: Yoshua Wuyts <yoshuawuyts@gmail.com>
@yoshuawuyts

This comment has been minimized.

Copy link
Member

yoshuawuyts commented Jan 11, 2019

PR is ready for review!

CI is currently blocked on futures-util-preview failing to build, which is relying on rust-lang-nursery/futures-rs#1406 or rust-lang-nursery/futures-rs#1407 being merged.

Created #115 to add back the access-control headers that were not included in this patch's scope.

Thanks!

@aturon

This comment has been minimized.

Copy link
Collaborator

aturon commented Apr 24, 2019

@tzilist are you interested in rebasing this PR?

@tzilist

This comment has been minimized.

Copy link
Contributor Author

tzilist commented May 3, 2019

@aturon sorry just saw this! I wish I had time, kinda in the middle of a crunch at work! If it's still up in a few weeks I'd be happy to!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.