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

swap functions for classes #1

Merged
merged 4 commits into from
Jan 8, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/__tests__/index.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ describe('request', () => {

expect(users).toBeDefined();
Object.keys(users).forEach((key) => {
const value = users[key];
const value = (users as any)[key];
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why as any? it should be an object. There wasn't any type issues here before

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was still blowing up, but I'll double check.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does this without that...

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was the issue I was running into. I fixed it by adding

[key:string]: () => Promise<User>

Which led to the original question.


expect(value).toBeInstanceOf(Function);
});
Expand Down
22 changes: 19 additions & 3 deletions src/__tests__/users.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,15 @@ import {
} from './__fixtures__';

import request from '../request';
import usersMethods from '../users';

const users = usersMethods(request);
import {UserApi} from '../users';

describe('users.me()', () => {
let users: UserApi;

beforeEach(() => {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we prefer multiple beforeEach or single instance for the tests

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably multiple in case there needs to be a different setup in one of the tests.

users = new UserApi(request);
});

it('calls fetch and calls fetch with appropriate defaults', async() => {
mockFetch(getMockResponse(MOCK_USERS_ME_RESPONSE_DATA));

Expand Down Expand Up @@ -61,6 +65,12 @@ describe('users.me()', () => {
});

describe('users.get(id)', () => {
let users: UserApi;

beforeEach(() => {
users = new UserApi(request);
});

it('calls fetch and calls fetch with appropriate defaults', async() => {
mockFetch(getMockResponse(MOCK_USERS_ME_RESPONSE_DATA));

Expand Down Expand Up @@ -106,6 +116,12 @@ describe('users.get(id)', () => {
});

describe('users.emailLookup(email)', () => {
let users: UserApi;

beforeEach(() => {
users = new UserApi(request);
});

it('calls fetch and calls fetch with appropriate defaults', async() => {
mockFetch(getMockResponse(MOCK_USERS_ME_RESPONSE_DATA));
const email = '[email protected]';
Expand Down
54 changes: 54 additions & 0 deletions src/baseApi.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
import {JSONRequest} from './types';

const SNAKE_CASE_MATCH = /_\w/g;
const snakeToCamel = (str: string) =>
str.replace(SNAKE_CASE_MATCH, (chars: string) => chars[1].toUpperCase());

const transformKeysSnakeToCamel = <T extends { [key: string]: any } = {}>(
obj: T
) =>
Object.keys(obj).reduce((memo, key) => {
let newValue = obj[key];
const camelKey = snakeToCamel(key);

if (
newValue &&
typeof newValue === 'object' &&
!Array.isArray(newValue)
) {
newValue = transformKeysSnakeToCamel(newValue);
}

return {
...memo,
[camelKey]: newValue,
};
}, {}) as T;

/**
* Returns a function that sends a request, and transforms its results
*/
const makeJsonRequest = <T>(
request: JSONRequest<T>,
transformers: Array<(obj: T) => T>
) => (url: string, options?: RequestInit) =>
request(url, options).then((response) =>
transformers.reduce<T>((acc, transformer) => {
let memo = acc;

memo = transformer(response);
return memo;
}, response)
);

/**
* Base API class for creating new API Classes.
* Also encapsulates default transformers such as snake to camel.
*/
export abstract class BaseApi<T> {
request: JSONRequest<T>;

constructor(req: JSONRequest<T>) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't do that because it needs to know which type you setup in the inherited class.

this.request = makeJsonRequest(req, [transformKeysSnakeToCamel]);
}
}
54 changes: 30 additions & 24 deletions src/index.ts
Original file line number Diff line number Diff line change
@@ -1,36 +1,42 @@
import {Sdk, SdkConfig, JSONRequest} from './types';
import request from './request';
import userMethods from './users';
import {UserApi} from './users';

export * from './constants';

const DEFAULT_API_URL = 'https://www.eventbriteapi.com/v3';

type MakeRequestFunction = <T>(
baseUrl: string,
token: string
) => JSONRequest<T>;

const makeRequest: MakeRequestFunction = (baseUrl: string, token: string) => (
endpoint,
options = {}
) => {
const url = `${baseUrl}${endpoint}`;
let requestOptions = options;

if (token) {
requestOptions = {
...requestOptions,
headers: {
...(requestOptions.headers || {}),
Authorization: `Bearer ${token}`,
},
};
}

return request(url, requestOptions);
};

const eventbrite = ({
baseUrl = DEFAULT_API_URL,
token,
}: SdkConfig = {}): Sdk => {
const requestHelper: JSONRequest = (endpoint, options = {}) => {
const url = `${baseUrl}${endpoint}`;
let requestOptions = options;

if (token) {
requestOptions = {
...requestOptions,
headers: {
...(requestOptions.headers || {}),
Authorization: `Bearer ${token}`,
},
};
}

return request(url, requestOptions);
};

return {
request: requestHelper,
users: userMethods(requestHelper),
};
};
}: SdkConfig = {}): Sdk => ({
request: makeRequest(baseUrl, token),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See notes about making this using the same function.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I see another note?

users: new UserApi(makeRequest(baseUrl, token)),
});

export default eventbrite;
10 changes: 5 additions & 5 deletions src/request.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {JSONRequest, JSONResponseData, ParsedResponseError} from './types';
import {JSONResponseData, ParsedResponseError} from './types';
import 'isomorphic-fetch';

/**
Expand Down Expand Up @@ -35,10 +35,10 @@ const _tryParseJSON = (res: Response): Promise<any> => {
* with our JSON API. Parses the JSON, provides appropriate headers, and asserts
* a valid status from the server.
*/
export const _fetchJSON = (
const _fetchJSON = <T>(
url: string,
{headers = {}, method = 'GET', mode, ...options}: RequestInit = {}
): Promise<{}> => {
): Promise<T> => {
let fetchHeaders = headers as HeadersInit;

if (method !== 'GET') {
Expand Down Expand Up @@ -145,7 +145,7 @@ const _catchStatusError = (res: Response): Promise<any> =>
* Low-level method that makes fetch requests, returning the response formatted as JSON.
* It parses errors from API v3 and throws exceptions with those errors
*/
const jsonRequest: JSONRequest = (url: string, options?: RequestInit) =>
_fetchJSON(url, options).catch(_catchStatusError);
const jsonRequest = <T>(url: string, options?: RequestInit) =>
_fetchJSON<T>(url, options).catch(_catchStatusError);

export default jsonRequest;
8 changes: 4 additions & 4 deletions src/types.ts
Original file line number Diff line number Diff line change
@@ -1,18 +1,18 @@
import {UserMethods} from './users';
import {UserApi} from './users';

export interface SdkConfig {
token?: string;
baseUrl?: string;
}

export type JSONRequest = (
export type JSONRequest<T = {}> = (
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it looks like sine this has a default, we could probably leave it off and it should work in most of the cases.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what happens when there's no default...

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I want us to keep the default. I was talking about the other places.

apiPath: string,
options?: RequestInit
) => Promise<{}>;
) => Promise<T>;

export interface Sdk {
request: JSONRequest;
users: UserMethods;
users: UserApi;
}

export interface ArgumentErrors {
Expand Down
82 changes: 28 additions & 54 deletions src/users.ts
Original file line number Diff line number Diff line change
@@ -1,66 +1,40 @@
import {JSONRequest} from './types';
import {BaseApi} from './baseApi';

export type Email = {
email: string;
primary: boolean;
};

export interface User {
id: string;
firstName: string;
lastName: string;
imageId: string;
email: Email[];
export interface Email {
email?: string;
primary?: boolean;
}

export interface UserMethods {
[key: string]: () => Promise<User>;
me: () => Promise<User>;
get: (id: string) => Promise<User>;
emailLookup: (email: string) => Promise<User>;
export interface User {
id?: string;
firstName?: string;
lastName?: string;
imageId?: string;
email?: Email[];
}

const SNAKE_CASE_MATCH = /_\w/g;
const snakeToCamel = (str: string) =>
str.replace(SNAKE_CASE_MATCH, (chars: string) => chars[1].toUpperCase());
/**
* API for working with Users
*/
export class UserApi extends BaseApi<User> {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks so much cleaner. 👍

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

async me() {
const response = await this.request('/users/me/');

const transformKeysSnakeToCamel = (obj: { [key: string]: any }): {} =>
Object.keys(obj).reduce((memo, key) => {
let newValue = obj[key];
const camelKey = snakeToCamel(key);
return response;
}

if (
newValue &&
typeof newValue === 'object' &&
!Array.isArray(newValue)
) {
newValue = transformKeysSnakeToCamel(newValue);
}
async get(id: string) {
const response = await this.request(`/users/${id}/`);

return {
...memo,
[camelKey]: newValue,
};
}, {});
return response;
}

export default (request: JSONRequest): UserMethods => {
const me = () =>
request('/users/me/').then(transformKeysSnakeToCamel) as Promise<User>;

const get = (id: string) =>
request(`/users/${id}/`).then(transformKeysSnakeToCamel) as Promise<
User
>;

const emailLookup = (email: string) =>
request('/users/lookup/', {
async emailLookup(email: string) {
const response = await this.request('/users/lookup/', {
method: 'POST',
body: JSON.stringify({email}),
}).then(transformKeysSnakeToCamel) as Promise<User>;
});

return {
me,
get,
emailLookup,
};
};
return response;
}
}