Navigation Menu

Skip to content
This repository has been archived by the owner on Jan 15, 2023. It is now read-only.

DefaultRequestTaskRunner does not handle IsAsync routes #183

Closed
jel-massih opened this issue Apr 15, 2020 · 3 comments
Closed

DefaultRequestTaskRunner does not handle IsAsync routes #183

jel-massih opened this issue Apr 15, 2020 · 3 comments

Comments

@jel-massih
Copy link

jel-massih commented Apr 15, 2020

It appears that the IsAsync flag is not used, so async ActionRoutes which only have ActionAsync set, have Action try to be called (which is null).

        public SampleController()
        {
            RegisterGetRequestAsync("/data", GetData);
        }

        public async Task<ChromelyResponse> GetData(ChromelyRequest request)
        {
              return new ChromelyResponse() { RequestId = request.Id, Data = "Hello"}; ;
        }

Might make sense to remove the async methods if the intention is to no longer support Async methods.

Got it working for project im working on in this cl, but definately super janky.
jel-massih@6925986

@mattkol
Copy link
Member

mattkol commented Apr 16, 2020

I cannot remember if this was missed at the time or intentional. But we should add it. I will look into adopting what you have done.

Also I noticed a bug on that.. you should be able to replace that with a custom implementation, but we are not replacing single instances with older registered.

This should be allowed:

    public class DemoChromelyApp : ChromelyBasicApp
    {
        public override void Configure(IChromelyContainer container)
        {
            base.Configure(container);
            --------
            container.RegisterSingleton(typeof(IChromelyRequestTaskRunner), typeof(IChromelyRequestTaskRunner).Name, typeof(CustomRequestTaskRunner));
        }
    }

I will look into that too.

Thanks for pointing it out.

mattkol pushed a commit that referenced this issue Apr 18, 2020
@mattkol
Copy link
Member

mattkol commented Apr 18, 2020

@jel-massih please let me know if you find anything with fix.
Planning a nuget update based on - #180, but hoping to include this too.

Between now and tomorrow is fine.
No hurry.

Thanks.

@jel-massih
Copy link
Author

Hey! That CL looks good to me, and Aync routes are working for me with the changes. Thanks!

Stelzi79 pushed a commit to Stelzi79/Chromely that referenced this issue Oct 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants