[WIP] Authentication context (resolves #94) #97
Conversation
willstott101
force-pushed
the
auth-context
branch
from
6e81d8e to
27a684c
Compare
willstott101
force-pushed
the
auth-context
branch
from
27a684c to
e71c370
Compare
| } | ||
| export class Git extends EventEmitter implements GitEvents { | ||
| export class Git<T = any> extends EventEmitter implements GitEvents<T> { |
There was a problem hiding this comment.
Can we add a comment on why this is any?
There was a problem hiding this comment.
Yeah, I'm not decided on this totally. I guess it could should probably default to undefined/void
Then TSC will warn you to add the type to your Git constructor if you return something from authenticate
well... no it won't it'll tell you that you can't return anything but you see what I mean
|
|
||
| export type ParsedGitRequest = ParsedServiceGitRequest | ParsedHeadGitRequest; | ||
|
|
||
| export function parseRequest(req: http.IncomingMessage): ParsedGitRequest { |
There was a problem hiding this comment.
This is a nice refactor, thank you!
src/util.test.ts
Outdated
| expect('').not.toEqual('should not have entered this callback'); | ||
| done(); | ||
| }); | ||
| expect(parseBasicAuth.bind(null, req)).toThrow(new BasicAuthError()); |
There was a problem hiding this comment.
can we also keep the status code asserts? We should make sure the response is formed correctly in the event of an error.
There was a problem hiding this comment.
yeah absolutely those are the kind of tests that have made me stay in WIP
|
I left a few comments, it will take time to go through this change more thoroughly. |
|
Sure, will address those a bit later today. Once using the library after this refactor... requiring throwing from |
Gitclass which allows theauthenticatefunction to respond with a context value (inspired by thecontextcallback inApolloServer).authenticatecalls).Aside from the changes to authenticate every request the re-factoring was mostly to help me reduce the number of code paths that need to be supported.
The more optionally-async optionally-promise callbacks we support the messier the code becomes and it was hard for me to be confident all code branches were correct.
That being said I'm happy to go backwards on some of this refactoring now that it all seems to work.
Left as a WIP as I want to expand the test suite to cover more error cases.