feat(request): Add support for SDK request method #8

benmvp · 2018-02-28T17:02:59Z

Adds sdk.request(url, fetchOptions) that can be used to make any API request without having a convience method for it.

It reads in the token & baseUrl properties that are passed in while creating the sdk object in order to generate the full URL.

Sample usage:

const eventbrite = require('eventbrite');

// Create configured Eventbrite SDK
const sdk = eventbrite({token: 'OATH_TOKEN_HERE'});

// See: https://www.eventbrite.com/developer/v3/endpoints/users/#ebapi-get-users-id
sdk.request('/users/me').then(res => {
    // handle response data
});

Also:

Renamed package from brite-rest to eventbrite
Updated README to reflect sample usage
Moved index.spec.ts into __tests__ folder
Had to disable camelcase ESLint rule because Prettier was unquoting non-conforming property names and the camelcase rule doesn't have fix functionality for prettier-eslint to fix
Added a bunch of eslint-plugin-typescript rules

NOTE: Per https://www.eventbrite.com/developer/v3/api_overview/authentication/#ebapi-authenticating-requests we should move the token into an authorization header instead of a query parameter, which is preferred. It'd also allow us to remove the url-lib dependency (and the one-off Typescript definition file).

Fixes #6

Adds `sdk.request(url, fetchOptions)` that can be used to make any API request without having a convience method for it. It reads in the `token` & `baseUrl` properties that are passed in while creating the `sdk` object in order to generate the full URL. Sample usage: ```js const sdk = require('eventbrite')({token: 'OATH_TOKEN_HERE'}); // See: https://www.eventbrite.com/developer/v3/endpoints/users/#ebapi-get-users-id sdk.request('/users/me').then(res => { // handle response data }); ``` Also: - Renamed package from `brite-rest` to `eventbrite` - Updated README to reflect sample usage - Moved `index.spec.ts` into `__tests__` folder - Had to disable `camelcase` ESLint rule because Prettier was unquoting non-conforming property names and the `camelcase` rule doesn't have fix functionality for prettier-eslint to fix NOTE: Per https://www.eventbrite.com/developer/v3/api_overview/authentication/#ebapi-authenticating-requests we should move the token into an authorization header instead of a query parameter, which is preferred. It'd also allow us to remove the `url-lib` dependency. Fixes #6.

kaylie-alexa · 2018-02-28T17:51:30Z

src/__tests__/index.spec.ts

+    });
+
+    afterEach(() => {
+        restoreMockFetch();


kaylie-alexa · 2018-02-28T17:56:42Z

src/request.ts

+ */
+export const checkStatus = (res: Response): Promise<Response> => {
+    if (res.status >= 300) {
+        return Promise.reject(res);


would it be better to throw an error here?

I think it makes sense to return a Promise because the type definition calls for it. If we didn't i'd imagine you might get you'd get an error like checkStatus.then is not a function

I would think so. This is how our current code works.

I'm thinking the reason an Error isn't thrown is in order to be able to pass the res. A vanilla Error only takes an error message so we wouldn't be able to pass the res and get out the info we need in later handlers.

I guess we could:

throw res;

but we really should only be throwing error objects...

What do you think?

@rwholey-eb checkStatus is used as a .then handler so anything anything thrown would just trigger a .catch. Technically I can just return res instead of returning Promise.resolve(res)

ah i misunderstood, carry on

Yeah it feels weird throwing an object. I was thinking the ideal would be returning Promise.reject(throw new Error(res.error)) but we're not guaranteed the response will contain an error and the current code may be using stuff from the raw response. Don't have a better solution yet :/

kaylie-alexa · 2018-02-28T17:58:43Z

src/request.ts

+
+const hasArgumentsError = (responseData: JSONResponseData): boolean =>
+    isObject(responseData['error_detail']) &&
+    isObject(responseData['error_detail']['ARGUMENTS_ERROR']);


I think you're looking for _.isPlainObject here?

Although I think in this case, both would actually work

kaylie-alexa · 2018-02-28T18:07:13Z

src/request.ts

+    let error = {
+        error: responseData.error,
+        description: responseData['error_description'],
+    } as ParsedResponseError;


as is my favorite part of Typescript so far

kaylie-alexa · 2018-02-28T18:07:52Z

src/request.ts

+        response
+            .json()
+            .then((responseData) => parseError(responseData))
+            .then((parsedError) => reject({response, parsedError}))


Is there a reason return both response and parsedError? can you do response: parsedError instead?

I'm assuming so that you can have access to the raw response as well as the parsedError. The response will have other meta data (like response code) that may be needed in addition to the parsed error.

This is also how the code currently is in core.

kaylie-alexa · 2018-02-28T18:11:44Z

src/request.ts

+        [propName: string]: any;
+    };
+}
+


what do you think about creating a separate file for type definitions?

Makes sense, but I'll have to see how it works. I wouldn't want to have to import from that separate file and then export again.

Seemed to work ok!

kaylie-alexa · 2018-02-28T18:13:04Z

README.md

+const sdk = require('eventbrite')({token: 'OATH_TOKEN_HERE'});
+
+// See: https://www.eventbrite.com/developer/v3/endpoints/users/#ebapi-get-users-id
+sdk.request('/users/me').then(res => {


so I think we shouldn't be encouraging people to use .request method directly and instead provide them wrappers for particular objects like getUser or getEvents so that when the endpoint changes the api kit will mask the change

I agree! I think it serves a purpose though and I think we should definitely make it available to our consumers. Maybe when the rest of our main apis get fleshed out we can move those to the forefront of our documentation, and note that they are slightly more stable and preferred.

I agree. When we have the wrappers this example will get replaced. But the nice thing is that someone (like core) could start using it now with just .request()

I imagine a use-case for .request() even when we do have the wrappers is if a new API is added and we haven't gotten around to building the wrapper for it. I believe it should always be made available in the SDK, just not in the forefront of docs

yup, exactly. Basically it's you're on your own method because we can change/deprecate the endpoints, whereas with the wrappers you're guaranteed to get some type of response back

rwholey-eb · 2018-02-28T19:21:53Z

src/request.ts

+ */
+export const checkStatus = (res: Response): Promise<Response> => {
+    if (res.status >= 300) {
+        return Promise.reject(res);


I think it makes sense to return a Promise because the type definition calls for it. If we didn't i'd imagine you might get you'd get an error like checkStatus.then is not a function

rwholey-eb · 2018-02-28T19:25:44Z

README.md

+const sdk = require('eventbrite')({token: 'OATH_TOKEN_HERE'});
+
+// See: https://www.eventbrite.com/developer/v3/endpoints/users/#ebapi-get-users-id
+sdk.request('/users/me').then(res => {


I agree! I think it serves a purpose though and I think we should definitely make it available to our consumers. Maybe when the rest of our main apis get fleshed out we can move those to the forefront of our documentation, and note that they are slightly more stable and preferred.

rwholey-eb · 2018-02-28T19:28:22Z

src/index.ts

@@ -1,5 +1,32 @@
-export interface Sdk {}
+/// <reference path="../definitions/url-lib.d.ts"/>


this is interesting, so this is like a require for ts definitions?

Yep, that's my understanding. Should be able to get rid of it with #9 though

As a result of this change I had to add `typescript/no-unused-vars` rule, so I went ahead and added a bunch more.

kaylie-alexa

nice work @benmvp !

ebtravis · 2018-06-22T22:20:50Z

🎉 This PR is included in version 1.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

BenAtEventbrite requested review from rwholey-eb and kaylie-alexa February 28, 2018 17:03

benmvp mentioned this pull request Feb 28, 2018

Token should be moved to authorization header #9

Closed

kaylie-alexa reviewed Feb 28, 2018

View reviewed changes

rwholey-eb reviewed Feb 28, 2018

View reviewed changes

Move types into their own file

b1bdbc0

As a result of this change I had to add `typescript/no-unused-vars` rule, so I went ahead and added a bunch more.

kaylie-alexa approved these changes Mar 1, 2018

View reviewed changes

BenAtEventbrite merged commit 149b0a1 into eventbrite:master Mar 1, 2018

benmvp deleted the request branch March 1, 2018 19:24

benmvp mentioned this pull request Mar 2, 2018

Add build targets #11

Closed

BenAtEventbrite added this to the Alpha milestone Apr 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(request): Add support for SDK request method #8

feat(request): Add support for SDK request method #8

benmvp commented Feb 28, 2018 •

edited

Loading

kaylie-alexa Feb 28, 2018

kaylie-alexa Feb 28, 2018

rwholey-eb Feb 28, 2018

benmvp Feb 28, 2018

benmvp Feb 28, 2018

rwholey-eb Feb 28, 2018

kaylie-alexa Feb 28, 2018

kaylie-alexa Feb 28, 2018

benmvp Feb 28, 2018

benmvp Mar 1, 2018

kaylie-alexa Feb 28, 2018

benmvp Feb 28, 2018

kaylie-alexa Feb 28, 2018

benmvp Feb 28, 2018

kaylie-alexa Feb 28, 2018

benmvp Feb 28, 2018

benmvp Mar 1, 2018

kaylie-alexa Feb 28, 2018

rwholey-eb Feb 28, 2018

benmvp Feb 28, 2018

benmvp Feb 28, 2018

kaylie-alexa Feb 28, 2018

rwholey-eb Feb 28, 2018

rwholey-eb Feb 28, 2018

rwholey-eb Feb 28, 2018

benmvp Feb 28, 2018

kaylie-alexa left a comment

ebtravis commented Jun 22, 2018

		@@ -1,5 +1,32 @@
		export interface Sdk {}
		/// <reference path="../definitions/url-lib.d.ts"/>

feat(request): Add support for SDK request method #8

feat(request): Add support for SDK request method #8

Conversation

benmvp commented Feb 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaylie-alexa left a comment

Choose a reason for hiding this comment

ebtravis commented Jun 22, 2018

benmvp commented Feb 28, 2018 •

edited

Loading