swap functions for classes #1
Conversation
| @@ -103,7 +103,7 @@ describe('request', () => { | |||
|
|
|||
| expect(users).toBeDefined(); | |||
| Object.keys(users).forEach((key) => { | |||
| const value = users[key]; | |||
| const value = (users as any)[key]; | |||
There was a problem hiding this comment.
why as any? it should be an object. There wasn't any type issues here before
There was a problem hiding this comment.
It was still blowing up, but I'll double check.
There was a problem hiding this comment.
It does this without that...
There was a problem hiding this comment.
That was the issue I was running into. I fixed it by adding
[key:string]: () => Promise<User>
Which led to the original question.
|
|
||
| describe('users.me()', () => { | ||
| let users: UserApi; | ||
|
|
||
| beforeEach(() => { |
There was a problem hiding this comment.
do we prefer multiple beforeEach or single instance for the tests
There was a problem hiding this comment.
Probably multiple in case there needs to be a different setup in one of the tests.
| export abstract class BaseApi<T> { | ||
| request: JSONRequest<T>; | ||
|
|
||
| constructor(req: JSONRequest<T>) { |
There was a problem hiding this comment.
Can we just make this JSONRequest<{}>? That way we don't have to double instance before and then the makeJsonRequest can take in a JSONRequest<{}> and then return a <T>. I really like the ideas of generics, but I feel like we may be over using it here a few times.
There was a problem hiding this comment.
You can't do that because it needs to know which type you setup in the inherited class.
| }; | ||
| }; | ||
| }: SdkConfig = {}): Sdk => ({ | ||
| request: makeRequest(baseUrl, token), |
There was a problem hiding this comment.
See notes about making this using the same function.
There was a problem hiding this comment.
Not sure I see another note?
|
|
||
| export interface SdkConfig { | ||
| token?: string; | ||
| baseUrl?: string; | ||
| } | ||
|
|
||
| export type JSONRequest = ( | ||
| export type JSONRequest<T = {}> = ( |
There was a problem hiding this comment.
Actually it looks like sine this has a default, we could probably leave it off and it should work in most of the cases.
There was a problem hiding this comment.
This is what happens when there's no default...
There was a problem hiding this comment.
Right, I want us to keep the default. I was talking about the other places.
| /** | ||
| * API for working with Users | ||
| */ | ||
| export class UserApi extends BaseApi<User> { |
|
Going to merge. We can transfer some of this conversation to the main PR. |
Just some ideas around using classes to reduce some duplication.