[Enhancement] Refactor handler(Raw|Http) calls to use Promises
pocesar opened this issue · 19 comments
To the implementators and extensions POV, refactoring a standard way using promises in those methods increase testability, interop and easier integrations. It also decouples the functionality that is currently ingrained inside those methods, and expose them for better unit testing
We could split this part into two, if you need help, to get some speed for reaching the milestone faster and focus on more improvements/refactoring/cleaning the code/testing.
What do you think, @pocesar ?
I'll push some of the things I've done so far, what is nagging me is the parser inside the handler methods
In what way ? what's the bugging part?
i synced the fork with this upstream, but I see no changes or commits.
I didn't push yet, it's a mess (only 4 tests passing), but if you want it anyway, I can push as-is
ok I put it in the develop branch, it's really broken right now. some places are creating internal classes inside, and expect them to handleMessage. I can either pass the conn to the promise result (would be better for control, and for integration) or I can just handle everything the way it is right now, and just resolve the promise when it's done (going to bed now)
I think I have some free time this weekend, after this I'm going to start working again. If you think you need a hand, just say, and I'll jump in.
Cheers! Onto the v2.0 of json-rpc2!
alright, if you can, I won't be able to work on it from today through sunday though. one thing, I removed the Promise.method from inside the checkAuth because it's an expensive call (it uses new Function under the hood), so having it in a frequently called function could have performance implications
What if we use it only once, when enabling the authorization handler type
(enableBasic/Cookie/JWTAuth) instead of every time we check the
authorization headers? I think this might solve the performance issue and
still be able to support synchronous implementations of the handler?
On Sat, Oct 8, 2016 at 1:47 AM, Paulo Cesar notifications@github.com
wrote:
alright, if you can, I won't be able to work on it from today through
sunday though. one thing, I removed the Promise.method from inside the
checkAuth because it's an expensive call (it uses new Function under the
hood), so having it in a frequently called function could have performance
implications—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#29 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABzKA2wdJmnvPw8iTTFmpaMW-dnNkLERks5qxsvmgaJpZM4KOByg
.
Would you mind if I try a clean implementation (from where I left off) and review it in a new pull request?
If not, I will try to continue from the last commit.
yeah, that's what I did, moved the Promise.method call inside the enable* functions (which could in turn become a private method that only does that this.authHandler = Promise.method(handler)
Okay, I was mistaken. If we set it inside the enable* functions once, I think it's enough. If we set it once during the enable functions, the setAuthType will do another unnecessary wrap each time we switch. I think we can remove that part. What is your opinion ?
any progress on this issue ?
I didn't make the time to continue your efforts for closing this one during the weekend.
nope, I'm having to finish off a platform for a startup with a tight deadline. will resume it hopefully by this weekend, but if you want to work on it, go ahead
I managed to get a working form for handleHttp and get the jsonrpc-test.js tests working (refactored them the 'promise way' i guess).
I might be able to make the other tests run until tomorrow evening (Bucharest time), too tired now. And with the authorization tests running, a pull request for the handleHttp part.
We'll have to see about handleRaw, don't want to handle it now :)
I managed to make the authorization tests run too. We have to make a discussion in the case the server and client have different auth types, what kind of error do we return (InternalError or InvalidParams with the error message provided in the auth handler). I checked if the message was UNAUTHORIZED (const) return InvalidParams, else return InternalError with the message provided by the developer in the handler.
(line 350 in the new pull request).
I will submit an early pull request to continue the discussion there for refactoring/improvements etc.
Thanks!
Are you going to take over the progress on the issue, including merge change requests and modifications? I've been uber busy these days, uni, work and bucharestjs hacks (on open data, first prize by the way ;) ) Let's have another talk on thursday (it's the first day I would be free), what do you say ?
I merged your PR so I would be able to continue on it (unless you made some new changes). I'm almost done with a sprint here for a private project and will resume working it in the meantime. Congrats on first prize!
Depending on the time the you're available, we can match the timezones (GMT-2 day time right now)
Hi @pocesar ,
Do you want to set up a time interval to have the talk ? If you are still up for it and have the time today.
I don't know how to reach you in a more immediate manner except github mentions. Gitter sends email notifications like github, so there is no difference, unless you install the mobile app.
Awaiting for your answer in, let's say, 2 hours (maximum 3)? It'll be the latest 10pm for me. Otherwise, schedule it for tomorrow and, if you have any changes, could you please push them on the remote ?
All the best,
Daniel.