Fill textarea with response from backend #109

amitmbee · 2018-04-24T12:11:08Z

revathskumar

After fetching the response we need to fill the textarea with the response. THis is only half done.

revathskumar · 2018-04-24T12:17:30Z

app/actions/index.ts

+      url: url
+    })
+    .then(response => response)
+      .then(response => dispatch(fetchSuccess(response)


we can avoid this second then.

If we are storing only response, the use response.data.
But I think we need to store the response type as well

revathskumar · 2018-04-24T12:18:09Z

app/actions/index.ts

+    .then(response => response)
+      .then(response => dispatch(fetchSuccess(response)
+    .catch(err => err)
+      .then(err => dispatch(fetchFailure(err)))


dispatch failure directly from catch

revathskumar · 2018-04-24T12:18:47Z

app/components/request_list.tsx

@@ -22,6 +22,7 @@ export interface RequestObj {
  disableInterceptor:React.ChangeEvent<HTMLButtonElement>;
  updateInterceptorStatus:React.ChangeEvent<HTMLButtonElement>;
  isInterceptorOn:object;
+  fetchResponse:React.MouseEvent<HTMLButtonElement>


How come this is React.MouseEvent ?

Since fetchResponse is defined on onClick event, it is supposed to be React.MouseEvent i feel. Ref :https://reactjs.org/docs/events.html#mouse-events

revathskumar · 2018-04-24T12:21:57Z

app/reducers/rootReducer.ts

      return {
        ...state,
        isInterceptorOn: {...state.isInterceptorOn, [action.payload.tabId]: action.payload.value}
      };
+    case actionType.FETCH_DATA_SUCCESS: {
+        return {...state, response : action.response};


we need to store response for each and every urls. THis is will overwrite the response when we fetch for second url

revathskumar · 2018-04-24T13:34:43Z

app/actions/index.ts

+      method: method,
+      url: url
+    })
+    .then(response => dispatch(response))


we can't just dispatch? where is the action?

revathskumar · 2018-04-24T13:34:54Z

app/actions/index.ts

+      url: url
+    })
+    .then(response => dispatch(response))
+    .catch(err => dispatch(err))


action missing

revathskumar · 2018-04-24T13:35:42Z

app/reducers/rootReducer.ts

+      ...state.responseData,
+      [action.payload.tabId]: {
+        ...state.responseData[action.payload.tabId],
+        [action.payload.index]: action.payload.error


why we are setting error in response data?

revathskumar · 2018-04-24T13:36:46Z

app/reducers/rootReducer.ts

+        return {...state, responseData : {
+          ...state.responseData,
+          [action.payload.tabId]: {
+            ...state.responseData[action.payload.tabId],


this is suppose to be requestId

revathskumar · 2018-04-24T13:37:48Z

app/actions/index.ts

+  return function (dispatch) {
+    axios({
+      method: method,
+      url: url


You need to pass the headers as well.

revathskumar · 2018-04-24T13:37:56Z

app/actions/index.ts

+}
+export function fetchResponse(url:string,method:string, tabId:number, index: number) {
+  return function (dispatch) {
+    axios({


return the promise

… after fetching responseText

amitmbee · 2018-05-02T03:39:11Z

app/components/Intercept_Components/InterceptTextBox.tsx

+                name="responseText"
+                className="responseText"
+                defaultValue={textAreaValue }
+                key={textAreaValue}


Added key attr to force re-render of textarea if changing defaultValue after clicking the fetchResponse (X) button

…button styles

revathskumar · 2018-05-02T05:03:17Z

README.md

-the extension will not mock any previously intercepted calls.
+The toggle switch is used to disable `INTERCEPTOR`. If the toggle is switched to `OFF` state, it displays a message saying `Interception Disabled` as below.
+
+<img src="images/interceptor_disabled.png" alt="Message shown by Interceptor on disabling">


these are already on master why this is showing as change here?

revathskumar · 2018-05-02T05:04:30Z

app/actions/index.ts

+export function fetchResponse(url:string,method:string, requestId:number) {
+  return {
+    type: FETCH_DATA,


this is suppose to be a async action right?

Yes, that is why we are using aliases - https://github.com/tshaddix/react-chrome-redux/wiki/Advanced-Usage#alias

revathskumar · 2018-05-02T06:00:28Z

app/components/Intercept_Components/InterceptTextBox.tsx

+          className="fetch-responsetext"
+          onClick={() => {
+            props.fetchResponse(props.rowProps.checkbox);
+            textAreaValue = JSON.stringify(props.responseData[props.rowProps.checkbox.requestId]) || responseTextValue;


this shouldn't be there. no need for || responseTextValue. thats should be handled in store.

responseTextValue holds the value that is propagated from onChange event of textarea. If there is nothing typed in the textarea, Then it holds a value of empty string. Here textAreaValue holds the value that is propagated from the store as props when the X(fetch response) button is clicked. We could handle this in a if else conditional, but not in a store I guess. Any other approach you can think of. Please refer : https://github.com/code-mancers/interceptor/blob/18a38363d3f4f4700cc6c6e40fa25f875f528d2d/app/components/Intercept_Components/InterceptTextBox.tsx

okey I understand the || responseTextvalue part. but still this statement should not be inside the onClick. This statement should be in the render method.

revathskumar · 2018-05-02T06:01:40Z

app/store/aliases.ts

+};
+
+export default {
+  FETCH_DATA: fetchDataAlias


please don't do this.
please export default fetchDataAlias

Did that according to the pattern defined here : https://github.com/tshaddix/react-chrome-redux/wiki/Advanced-Usage#alias . Should I change it ?

This is confusing can we use the normal thing since we are not integrating anything with background.

kgrz · 2018-05-04T08:32:48Z

package.json

@@ -44,6 +44,7 @@
    "test:watch": "jest --watch"
  },
  "dependencies": {
+    "@types/axios": "^0.14.0",


I'm not sure why these got updated, may be deliberate choice, but if not, ensure these changes don't happen. These can be caused due to running npm upgrade instead of npm install (or yarn equivalent)

kgrz · 2018-05-04T08:34:09Z

app/store/aliases.ts

+        dispatch(fetchSuccess(data, requestId));
+        dispatch(handleRespTextChange(data.data, requestId))
+      })
+      .catch(err => {


try to avoid catch when not required. The second argument to then here has the same affect as what catch does. using a .catch handler would mean that any JS errors or other errors also get caught. Not a huge problem here, but that can become a habit

The catch block here is executed in case the axios lib isn't able to complete and fetch the response. Don't you recoemmend using this in case the server returns a error such as 404 or 500. Refer this

in case of axios rejecting for any reason (be it a problem with axios itself, or may be a network call failed), the second argument of .then gets called. There are various reasons why they might've suggested using a catch block but IME, it's a good practice to not abuse catch.

kgrz · 2018-05-04T08:34:40Z

app/store/aliases.ts

+const fetchDataAlias = (payload: object) => {
+  return dispatch => {
+    const {method, url, requestId, requestHeaders} = payload.requestDetails;
+    const requestHeadersObject = requestHeaders.reduce((accumulatedObj, reqHeaderNameValuePair) => {


can you explain what's the intent of this?

In axios, if we need to pass the request headers while making a request, we need to specify it this way headers: {'X-Requested-With': 'XMLHttpRequest'},. But from the background script, we get the headers in the following format.

requestHeaders : { [0] : {name : 'X-Requested-With' , value : 'XMLHttpRequest'}, [1] : ... } So the function returns the headers in the way that axios requires. Ref: https://github.com/axios/axios#request-config

kgrz · 2018-05-04T08:38:53Z

app/components/Intercept_Components/InterceptTextBox.tsx

@@ -2,12 +2,19 @@ import * as React from "react";
 import {RequestHeaderList} from "./RequestHeaderList";

 export const InterceptTextBox = props => {
+  const isObject = (val:any) => {


A simpler check for this would be:

return !value && typeof val === 'object';

So that would return true only if the value is truthy, and it's a type of object. null is also an object, but that's covered in the first check. stringify of a function is pretty much a noop, so that check is not required, we can treat it as not an object.

kgrz · 2018-05-04T08:40:39Z

app/components/Intercept_Components/InterceptTextBox.tsx

-  const responseTextValue = props.responseText[props.rowProps.checkbox.requestId] || defaultResponseText;
-  const statusCodeValue = props.statusCodes[props.rowProps.checkbox.requestId] || defaultStatusCode;
-  const contentTypeValue = props.contentType[props.rowProps.checkbox.requestId] || defaultContentType;
+  const responseTextValue = isObject(props.responseText[requestId]) ? JSON.stringify(props.responseText[requestId]) : props.responseText[requestId] || defaultResponseText;


Ideally, it's more cleaner and easy to follow if the type of data is consistent: either always ensure it's an object, or ensure it's always a string (at this function call site). let the caller do the processing. That way, your main interception code doesn't need to handle all kinds of complex marshalling logic.

kgrz · 2018-05-04T08:41:18Z

app/components/Intercept_Components/InterceptTextBox.tsx

-        <label htmlFor="">Response Text</label>
+        <label className="responseTextlabel">Response Text</label>
+        <span
+          title="Fetch Response Text"


why not use a <a> or a button tag for this? Also, title here doesn't seem to suggest what the action does

Yeah, wanted that to replace that with an icon before the release. Just put up a placeholder span tag for now

kgrz · 2018-05-04T08:41:54Z

app/store/popup_store.ts

 import { createLogger } from 'redux-logger'
 import { Middleware } from 'react-redux/node_modules/redux';
+import thunkMiddleware from 'redux-thunk';
+import aliases from './aliases'
+import { alias } from 'react-chrome-redux';


what does this do?

Okay, more precisely, why do you want to use this?

nevermind, I think I understand.

…nent

…ating with values from server

revathskumar

Where are we showing the error message when fetch fails?

revathskumar · 2018-05-09T09:46:30Z

app/components/Intercept_Components/InterceptTextBox.tsx

-  const responseTextValue = props.responseText[props.rowProps.checkbox.requestId] || defaultResponseText;
-  const statusCodeValue = props.statusCodes[props.rowProps.checkbox.requestId] || defaultStatusCode;
-  const contentTypeValue = props.contentType[props.rowProps.checkbox.requestId] || defaultContentType;
+  let responseTextValue = props.responseText[requestId] || defaultResponseText;


why let. can be const

revathskumar · 2018-05-09T09:47:09Z

app/components/Intercept_Components/InterceptTextBox.tsx

+          onClick={() => {
+            props.fetchResponse(props.rowProps.checkbox)
+        }
+      }


fix indentation.

revathskumar · 2018-05-09T09:51:20Z

app/store/aliases.ts

+        dispatch(fetchSuccess(JSON.stringify(data.data), requestId));
+      })
+      .catch(err => {
+        dispatch(fetchFailure("", requestId))


error message can't be blank.

revathskumar · 2018-05-09T09:51:39Z

app/store/aliases.ts

+      .then(data => {
+        dispatch(fetchSuccess("", requestId))
+        dispatch(handleRespTextChange(JSON.stringify(data.data), requestId))
+        dispatch(fetchSuccess(JSON.stringify(data.data), requestId));


avoid repeated calls of JSON.stringify

amitmbee added 2 commits April 24, 2018 16:00

Setup redux-thunk, actions to fetch response on button click

4b598db

Define action creators and reducer for data fetch success and failure

4efb204

emilsoman added the ready for review label Apr 24, 2018

revathskumar suggested changes Apr 24, 2018

View reviewed changes

Modify reducer to store response data for each tabId and each request

5464d12

revathskumar suggested changes Apr 24, 2018

View reviewed changes

amitmbee added 7 commits April 30, 2018 15:09

Use aliases, requestId instead of tabId

c3e3663

Setup redux-thunk, actions to fetch response on button click

2bcba47

Define action creators and reducer for data fetch success and failure

b130936

Modify reducer to store response data for each tabId and each request

52391ef

Use aliases, requestId instead of tabId

24c909a

Add key attr to force re-render of textarea after seting defaultValue…

9910bec

… after fetching responseText

Add key attr to force re-render of textarea after seting defaultValue…

7c729be

… after fetching responseText

amitmbee commented May 2, 2018

View reviewed changes

amitmbee added 2 commits May 2, 2018 11:21

Send axios request to fetch response with request-headers, Fix fetch …

18a3836

…button styles

Specify proper TS types for mouseEvents

6c2b626

revathskumar suggested changes May 2, 2018

View reviewed changes

amitmbee force-pushed the Fill-textarea-with-response branch from 3e6ebe8 to 6c2b626 Compare May 2, 2018 11:20

amitmbee added 3 commits May 3, 2018 19:29

Use a single value from store to update textarea defaultValue

ccf1a7b

Use aliases to dispatch async actions instead of in the component

d8cf415

Log whenever the compoent re-renders

e1318f2

kgrz reviewed May 4, 2018

View reviewed changes

amitmbee added 6 commits May 7, 2018 13:17

Send stringified data to the reducer instead of checking in the compo…

d8146e9

…nent

Empty the responseData and responseText values from store, before upd…

32297e9

…ating with values from server

Style fetch response button

b366eef

Merge branch 'master' into Fill-textarea-with-response

e5ba3f1

Update yarn lockfile

c4891d6

Add bg-icon to fetch-response button

4ff5509

revathskumar suggested changes May 9, 2018

View reviewed changes

amitmbee and others added 9 commits May 9, 2018 16:19

Show a error message if if axios returns a error for fetch call

28e2f3f

Use fetch icon as bg within fetch response button

17c0717

Remove used icons from images folder

06a161f

Update readme : Add credits to The Noun Project for the icons used

b8f6963

Display Attribution title in readme properly

41808b3

Update Readme - Add fetch response section

ceec4a8

Remove un-needed package from actions.ts

11d9ece

Minify readme images

57c6999

use icon only for fetch response button

04f8f6b

revathskumar approved these changes May 14, 2018

View reviewed changes

revathskumar merged commit 7a76114 into master May 14, 2018

revathskumar deleted the Fill-textarea-with-response branch May 14, 2018 20:49

Fill textarea with response from backend #109

Fill textarea with response from backend #109

Conversation

amitmbee commented Apr 24, 2018

revathskumar left a 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

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

revathskumar left a 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