-
Notifications
You must be signed in to change notification settings - Fork 7
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
Michael/perf 1699 #270
Michael/perf 1699 #270
Conversation
Already approved |
@blonde-mike Testing this locally it doesn't seem to be working. I'm getting CORS header errors making the requests. on a few test urls. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@blonde-mike I added some comments to the PR.
|
||
interface RequestDetailsTabsProps extends HeadersViewProps, ResponseViewProps {} | ||
|
||
function RequestDetailsTabs ({ id, headersList, removeHeader, changeHeader, addHeader, responseCode, response }: RequestDetailsTabsProps): JSX.Element { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When creating sub elements, it's a bit cleaner to put them in their own file and create a separate storybook for them.
addHeader: (headerType?: HeaderType) => void; | ||
} | ||
|
||
function HeadersView ({ id, headersList, removeHeader, changeHeader, addHeader }: HeadersViewProps): JSX.Element { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When creating sub elements, it's a bit cleaner to put them in their own file and create a separate storybook for them.
}; | ||
try { | ||
const response = await axios(request); | ||
setLastResponseCode(response.status + " " + response.statusText); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're using multiple useState
hooks together it's more efficient to use a single state object and set them at the same time. Ex. setLastResponse({ data: response.data, status: response.status + " " + response.statusText })
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's a guide for when to merge vs. keeping them separate.
https://legacy.reactjs.org/docs/hooks-faq.html#should-i-use-one-or-many-state-variables
setLastResponse(response.data); | ||
return response.data; | ||
} catch (error) { | ||
throw error; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Throwing the error are we displaying the error to the user somehow? Locally I'm having to look at the console to see the CORS errors I was getting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(error as AxiosError).isAxiosError
will tell you if it's an AxiosError
type.
acc[header.name] = header.value; | ||
return acc; | ||
}, {} as Record<string, string>), | ||
validateStatus: () => true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably should actually look at the status code. There's other errors like CORS errors that are not being displayed properly
* Fix multiple test results bug (#269) * Fixed the bug with multiple results files and switching between - The null pointer throws on redraw. All the calculations and freeing of memory must be done within a setState block while the data is static * Updated eslint to include storybook * Updated eslint to include storybook * Fixed eslint files * Michael/perf 1699 (#270) * Updated Request tool to get it online * Removed unused component * Updates to remove lint errors * PewPew improvements (#271) * PewPew improvements * Updated scripts for running locally * Remove har to yaml from controller (#272) * Removed the yamlwriter and all its components from the controller * Changed the yamlwriter link in the layout to go to the guide yaml writer * Removed Non Prod message for login * Removed unneeded vars --------- Co-authored-by: blonde-mike <[email protected]>
Cleaned Edit URL modal and got endpoint Test button online. Does not yet handle request body or url params, but can make a basic GET request and display the results.