Collect all frontend api requests in one place #311

simonbtomlinson · 2018-03-04T03:07:17Z

Pulls all api requests into a single Api.ts file that takes care of things like authentication.
I'd be happy to receive feedback about how Api.ts can be split up in a way that's easier to read while still easy to use.

This required modifying some server-side validation code in places that expected a token parameter. The token can be passed in a number of ways (see src/server/routes/authenticator.js for the relevant code), but lots of validation code assumed it would be passed in a specific way.

This removes validation for the token in the groups routes because the token is not actually necessary (it might be passed by a header) and in fact the Api class exclusively passes it as a header.

Splits the api into an `ApiBackend` that handles actual networking and a class for each category of api actions. For example, `MetersApi` contains methods for interacting with meters.

simonbtomlinson · 2018-03-04T04:25:21Z

I've separated the parts of the Api class in commit 5b5b891.

johndiiorio · 2018-03-04T20:56:30Z

src/client/app/actions/admin.ts

 import { ChartTypes } from '../types/redux/graph';
 import { PreferenceRequestItem } from '../types/items';
 import * as t from '../types/redux/admin';
 import { ActionType, Dispatch, GetState, Thunk } from '../types/redux/actions';
 import { State } from '../types/redux/state';
+import {preferencesApi} from '../utils/api';


We aren't using this spacing style in the project. Perhaps we need to add a tslint rule?

I don't think there's a tslint rule for this. I do like { foo } style better though, and a rule would be nice. See here: palantir/tslint#1044. I'll fix it manually.

johndiiorio · 2018-03-04T20:59:06Z

src/client/app/utils/api/ApiBackend.ts

+ *
+ * Should probably only be used by other api classes.
+ */
+class ApiBackend {


I don't like the name ApiBackend, as it signifies something server-side. How about ApiInternal.ts, Api.ts, or ApiDispatcher.ts?

johndiiorio · 2018-03-04T21:00:38Z

src/client/app/utils/api/GroupsApi.ts

+	}
+
+	public async delete(groupID: number) {
+		return await this.backend.doPostRequest('api/groups/delete', {id: groupID});


Why is this a POST instead of DELETE request? Recommend changing the server to correspond to be a DELETE request.

I didn't actually change the way we do any networking, as I didn't think it was in-scope for this pr. We can definitely change that in a different pr though.

Yeah, I'm on board for changing this in a future PR.

johndiiorio · 2018-03-04T21:04:54Z

src/client/app/components/AdminComponent.tsx

-					token: getToken()
-				}
-			})
+			metersApi.submitNewReadings(this.props.selectedImportMeter.value, file)


Is there a reason directly below this line that you used a .then instead of async/await? Or would that get ugly fast because await needs to be in an async function?

It's cleaner than wrapping it in an async IIFE in this case, and it was already like that, so I decided not to change it.

johndiiorio · 2018-03-04T21:09:48Z

src/client/app/components/LoginComponent.tsx

-				// If there was a problem other than a lack of authorization, the user can't fix it.
-				// Log it to the console for developer use.
-				console.error(err); // tslint:disable-line no-console
+		(async () => {


Would it be possible to mark handleSubmit as async instead of using an async IFFE? Or does that not work with React?

I can't call setState() inside an async function, so this has to stay an IFFE.

johndiiorio · 2018-03-04T21:41:13Z

src/server/routes/groups.js

@@ -110,9 +110,8 @@ router.post('/create', async (req, res) => {
 	const validGroup = {
 		type: 'object',
 		maxProperties: 4,


Don't forget to change maxProperties here and on all the other validation routes you changed.

johndiiorio · 2018-03-04T21:47:02Z

src/client/app/components/LoginComponent.tsx

+				} else {
+					// If there was a problem other than a lack of authorization, the user can't fix it.
+					// Log it to the console for developer use.
+					console.error(err); // tslint:disable-line no-console


Another use of console.eror -> throw new Error

johndiiorio · 2018-03-04T21:48:38Z

src/client/app/utils/api/ApiBackend.ts

+/**
+ * Handles the actual sending and receiving of network requests.
+ *
+ * Should probably only be used by other api classes.


Recommend removing "probably", we definitely don't want it used outside other API classes.

johndiiorio · 2018-03-04T21:51:13Z

src/client/app/utils/api/ApiBackend.ts

+		headers: { [key: string]: string},
+		extraConfig: AxiosRequestConfig
+	): AxiosRequestConfig {
+		if (hasToken()) {


Doesn't this always send a token regardless of the route? Is this best practice?

It does. I think this is a good way to do it. It makes things available to the server more often and there's less of a downside. We can just make the server send a 401 unauthorized if the user doesn't have the appropriate authorization level.

johndiiorio · 2018-03-04T21:54:06Z

src/client/app/actions/admin.ts

-							dispatch2(changeBarStacking());
-						}
-					});
+		const preferences = await preferencesApi.getPreferences();


I know the old code did not have a catch condition, but should we be explicit and add a catch to every request?

Maybe? I feel like we want to throw an error here, at least until we have more robust error handling (which isn't in scope for this pr, I don't think).

buip

I don't know about the client side but the server side looks good to me!

buip

Sorry for the last comment! I did not look carefully

buip · 2018-03-08T03:07:11Z

src/server/routes/groups.js

@@ -157,9 +156,8 @@ router.put('/edit', async (req, res) => {
 	const validGroup = {
 		type: 'object',
 		maxProperties: 5,


I think maxProperties should be 4 here

buip · 2018-03-08T03:07:31Z

src/server/routes/groups.js

@@ -227,9 +225,8 @@ router.post('/delete', async (req, res) => {
 	const validParams = {
 		type: 'object',
 		maxProperties: 2,


I think maxProperties should be 1 here

simonbtomlinson added 12 commits February 27, 2018 17:08

Begin refactoring all network requests to Api

c9b203f

Remove extra mpl headers

93585a5

Extract barchart network requests to api

5c37373

Api for meter details

f314f63

Api for groups actions

f04a0f0

This removes validation for the token in the groups routes because the token is not actually necessary (it might be passed by a header) and in fact the Api class exclusively passes it as a header.

Fix lint error

40c4c18

Api for preferences

bbd4338

Merge branch 'development' into extract-api

e8f8a48

Extract token checking functionality to the api

948d19c

Extract login functionality to the api

6c8f526

Remove unused axios import

db14ba4

Extract reading imports to the api

66bf3cc

simonbtomlinson requested review from NoraCodes, jameeters, sandeepacharya464, johndiiorio and buip March 4, 2018 03:07

Seperate parts of the api

5b5b891

Splits the api into an `ApiBackend` that handles actual networking and a class for each category of api actions. For example, `MetersApi` contains methods for interacting with meters.

johndiiorio suggested changes Mar 4, 2018

View reviewed changes

simonbtomlinson added 4 commits March 4, 2018 17:43

Fix formatting for imports

863da78

Fix incorrect maxProperties in groups route

cee7f3c

Fix formatting in api classes

28a6da3

Improve error handling for network requests

8f81c09

buip approved these changes Mar 8, 2018

View reviewed changes

buip suggested changes Mar 8, 2018

View reviewed changes

Fix incorrect maxProperties for groups routes

9d40f7e

buip approved these changes Mar 15, 2018

View reviewed changes

johndiiorio approved these changes Mar 19, 2018

View reviewed changes

simonbtomlinson merged commit 6f70e6c into development Mar 19, 2018

simonbtomlinson deleted the extract-api branch March 19, 2018 19:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Collect all frontend api requests in one place #311

Collect all frontend api requests in one place #311

simonbtomlinson commented Mar 4, 2018

simonbtomlinson commented Mar 4, 2018

johndiiorio Mar 4, 2018

simonbtomlinson Mar 4, 2018

johndiiorio Mar 4, 2018

johndiiorio Mar 4, 2018

simonbtomlinson Mar 4, 2018

NoraCodes Mar 6, 2018

johndiiorio Mar 4, 2018

simonbtomlinson Mar 6, 2018

johndiiorio Mar 4, 2018

simonbtomlinson Mar 6, 2018

johndiiorio Mar 4, 2018

johndiiorio Mar 4, 2018

johndiiorio Mar 4, 2018

johndiiorio Mar 4, 2018

simonbtomlinson Mar 4, 2018

johndiiorio Mar 4, 2018

simonbtomlinson Mar 4, 2018

buip left a comment

buip left a comment

buip Mar 8, 2018

buip Mar 8, 2018

Collect all frontend api requests in one place #311

Collect all frontend api requests in one place #311

Conversation

simonbtomlinson commented Mar 4, 2018

simonbtomlinson commented Mar 4, 2018

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

buip left a comment

Choose a reason for hiding this comment

buip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment