-
Notifications
You must be signed in to change notification settings - Fork 219
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
feat: RequestSamples in docs #864
Changes from 3 commits
ae7babf
30c10a5
0e53184
e1cd84c
79e9d56
bf769a9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export * from './RequestSamples'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ import { safeStringify } from '@stoplight/json'; | |
import { Button, Flex, Panel, Text } from '@stoplight/mosaic'; | ||
import { CodeViewer } from '@stoplight/mosaic-code-viewer'; | ||
import { Dictionary, IHttpOperation, IMediaTypeContent } from '@stoplight/types'; | ||
import { Request as HarRequest } from 'har-format'; | ||
import * as Sampler from 'openapi-sampler'; | ||
import * as React from 'react'; | ||
|
||
|
@@ -35,6 +36,12 @@ export interface TryItProps { | |
* Only applies when `showMocking` is enabled | ||
*/ | ||
mockUrl?: string; | ||
|
||
/** | ||
* Callback to retrieve the current request in a HAR format. | ||
* Called whenever the request was changed in any way. Changing `httpOperation`, user entering parameter values, etc. | ||
*/ | ||
onRequestChange?: (currentRequest: HarRequest) => void; | ||
} | ||
|
||
interface ResponseState { | ||
|
@@ -50,7 +57,7 @@ interface ErrorState { | |
* Displays the TryIt component for a given IHttpOperation. | ||
* Relies on jotai, needs to be wrapped in a PersistenceContextProvider | ||
*/ | ||
export const TryIt: React.FC<TryItProps> = ({ httpOperation, showMocking, mockUrl }) => { | ||
export const TryIt: React.FC<TryItProps> = ({ httpOperation, showMocking, mockUrl, onRequestChange }) => { | ||
const [response, setResponse] = React.useState<ResponseState | ErrorState | undefined>(); | ||
const [loading, setLoading] = React.useState<boolean>(false); | ||
const server = httpOperation.servers?.[0]?.url; | ||
|
@@ -82,22 +89,36 @@ export const TryIt: React.FC<TryItProps> = ({ httpOperation, showMocking, mockUr | |
|
||
const [textRequestBody, setTextRequestBody] = React.useState<string>(''); | ||
|
||
React.useEffect(() => { | ||
let isActive = true; | ||
if (onRequestChange) { | ||
buildHarRequest({ | ||
mediaTypeContent, | ||
parameterValues: parameterValuesWithDefaults, | ||
httpOperation, | ||
bodyInput: formDataState.isFormDataBody ? bodyParameterValues : textRequestBody, | ||
}).then(request => { | ||
if (isActive) onRequestChange(request); | ||
}); | ||
} | ||
return () => { | ||
isActive = false; | ||
}; | ||
// disabling because we don't want to react on `onRequestChange` change | ||
// eslint-disable-next-line react-hooks/exhaustive-deps | ||
}, [httpOperation, parameterValuesWithDefaults, formDataState.isFormDataBody, bodyParameterValues, textRequestBody]); | ||
|
||
if (!server) return null; | ||
|
||
const handleClick = async () => { | ||
try { | ||
setLoading(true); | ||
const mockData = getMockData(mockUrl, httpOperation, mockingOptions); | ||
const shouldIncludeBody = ['PUT', 'POST', 'PATCH'].includes(httpOperation.method.toUpperCase()); | ||
const request = await buildFetchRequest({ | ||
parameterValues: parameterValuesWithDefaults, | ||
httpOperation, | ||
mediaTypeContent, | ||
bodyInput: shouldIncludeBody | ||
? formDataState.isFormDataBody | ||
? bodyParameterValues | ||
: textRequestBody | ||
: undefined, | ||
bodyInput: formDataState.isFormDataBody ? bodyParameterValues : textRequestBody, | ||
mockData, | ||
}); | ||
const response = await fetch(...request); | ||
|
@@ -204,6 +225,7 @@ async function buildFetchRequest({ | |
mockData, | ||
}: BuildFetchRequestInput): Promise<Parameters<typeof fetch>> { | ||
const server = mockData?.url || httpOperation.servers?.[0]?.url; | ||
const shouldIncludeBody = ['PUT', 'POST', 'PATCH'].includes(httpOperation.method.toUpperCase()); | ||
philsturgeon marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
const queryParams = httpOperation.request?.query | ||
?.map(param => [param.name, parameterValues[param.name] ?? '']) | ||
|
@@ -213,9 +235,12 @@ async function buildFetchRequest({ | |
const url = new URL(server + expandedPath); | ||
url.search = new URLSearchParams(queryParams).toString(); | ||
|
||
const body = typeof bodyInput === 'object' ? await createRequestBody(httpOperation, bodyInput) : bodyInput; | ||
|
||
return [ | ||
url.toString(), | ||
{ | ||
credentials: 'omit', | ||
method: httpOperation.method, | ||
headers: { | ||
'Content-Type': mediaTypeContent?.mediaType ?? 'application/json', | ||
|
@@ -224,11 +249,67 @@ async function buildFetchRequest({ | |
), | ||
...mockData?.header, | ||
}, | ||
body: typeof bodyInput === 'object' ? await createRequestBody(httpOperation, bodyInput) : bodyInput, | ||
body: shouldIncludeBody ? body : undefined, | ||
}, | ||
]; | ||
} | ||
|
||
async function buildHarRequest({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function requires a lot of cleanup, I know. And potentially a move to another file along with |
||
httpOperation, | ||
bodyInput, | ||
parameterValues, | ||
mediaTypeContent, | ||
}: BuildFetchRequestInput): Promise<HarRequest> { | ||
const server = httpOperation.servers?.[0]?.url; | ||
const mimeType = mediaTypeContent?.mediaType ?? 'application/json'; | ||
const shouldIncludeBody = ['PUT', 'POST', 'PATCH'].includes(httpOperation.method.toUpperCase()); | ||
|
||
const queryParams = httpOperation.request?.query | ||
?.map(param => ({ name: param.name, value: parameterValues[param.name] ?? '' })) | ||
.filter(({ value }) => value.length > 0); | ||
|
||
let postData: HarRequest['postData'] = undefined; | ||
if (shouldIncludeBody && typeof bodyInput === 'string') { | ||
postData = { mimeType, text: bodyInput }; | ||
} | ||
if (shouldIncludeBody && typeof bodyInput === 'object') { | ||
postData = { | ||
mimeType, | ||
params: Object.entries(bodyInput).map(([name, value]) => { | ||
if (value instanceof File) { | ||
return { | ||
name, | ||
fileName: value.name, | ||
contentType: value.type, | ||
}; | ||
} | ||
return { | ||
name, | ||
value, | ||
}; | ||
}), | ||
}; | ||
} | ||
|
||
return { | ||
method: httpOperation.method.toUpperCase(), | ||
url: server + uriExpand(httpOperation.path, parameterValues), | ||
httpVersion: 'HTTP/1.1', | ||
cookies: [], | ||
headers: [ | ||
{ name: 'Content-Type', value: mimeType }, | ||
...(httpOperation.request?.headers?.map(header => ({ | ||
name: header.name, | ||
value: parameterValues[header.name] ?? '', | ||
})) ?? []), | ||
], | ||
queryString: queryParams ?? [], | ||
postData: postData, | ||
headersSize: -1, | ||
bodySize: -1, | ||
}; | ||
} | ||
|
||
function uriExpand(uri: string, data: Dictionary<string, string>) { | ||
if (!data) { | ||
return uri; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,12 @@ | ||
import { NodeType } from '@stoplight/types'; | ||
import * as React from 'react'; | ||
|
||
import { DocsSkeleton } from '../components/Docs'; | ||
import { TryIt as TryItComponent } from '../components/TryIt'; | ||
import { useParsedData } from '../hooks/useParsedData'; | ||
import { useActionsApi, usePlatformApi } from '../hooks/usePlatformApi'; | ||
import { BundledBranchNode } from '../types'; | ||
import { ActiveInfoContext } from './Provider'; | ||
import { ActiveInfoContext } from '../../containers/Provider'; | ||
import { useParsedData } from '../../hooks/useParsedData'; | ||
import { useActionsApi, usePlatformApi } from '../../hooks/usePlatformApi'; | ||
import { BundledBranchNode } from '../../types'; | ||
import { DocsSkeleton } from '../Docs'; | ||
import { TryItWithRequestSamples } from './TryItWithRequestSamples'; | ||
|
||
export interface ITryItProps { | ||
className?: string; | ||
|
@@ -18,7 +18,7 @@ type MockUrlResult = { servicePath: string; operationPath?: string; id: number } | |
const bundledNodesUri = 'api/v1/projects/{workspaceSlug}/{projectSlug}/bundled-nodes/{uri}'; | ||
const mockActionsUrl = 'api/actions/branchNodeMockUrl'; | ||
|
||
export const TryIt = ({ node }: ITryItProps) => { | ||
export const TryItContainer = ({ node }: ITryItProps) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I moved it next to the rest of
|
||
const info = React.useContext(ActiveInfoContext); | ||
|
||
const nodeUri = node || info.node; | ||
|
@@ -62,5 +62,5 @@ export const TryIt = ({ node }: ITryItProps) => { | |
|
||
if (parsedData?.type !== NodeType.HttpOperation) return null; | ||
|
||
return <TryItComponent httpOperation={parsedData.data} showMocking mockUrl={mockUrlResult?.servicePath} />; | ||
return <TryItWithRequestSamples httpOperation={parsedData.data} showMocking mockUrl={mockUrlResult?.servicePath} />; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
import { Box } from '@stoplight/mosaic'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I worry about this inheritence approach over some other form of linking things up, and maybe I'm worrying wrong here, but it seems like when we add Response Sample we might end up with TryItWithRequestSamplesAndResponseExamples.tsx? If there's a plan for response examples which im not aware of that will help us avoid that, then no biggy. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not inheritance, it's composition. And yes, that's what we'll end up with, and we'll likely call it My naming is non-ideal, that's a given. Feel free to suggest another. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The name is not the concern, the concern is that the only way our different sub components can talk to each other is by making more and more super components. Whether the right term in TS is inheritance or composition, the problem here is Foo,FooAndBar, FooAndBarAndBaz,BazAndFoo,FooAndBaz, FooAllTheThings. There have to be some alternative approaches to getting request samples into the api component that don’t include bunging it into Try It just so it has access to some of the data try it is creating, like the request it’s building. Do components not have any other way to share information? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm still not sure I get your point. There are other ways, but the most elementary and go-to way you make react components communicate with eachother is via super components, by lifting state up. You learn this lesson while completing the official React tutorial The other the approaches (state management libraries, custom hooks, HOCs) are more advanced and to be used when the basic one is impractical. You don't have to build one for each combination though. If you need a
It is not bunging into it, it is creating a component that holds them together. I could have pulled the state up all the way to SidebarLayout & StackedLayout & TryItContainer, then reduced the code-duplication with a custom hook, but that's an inferior solution IMO. (It still leaves some duplication in the components, the usage of the hook and the JSX stacking TryIt and RequestSamples on top of eachother, and it's also in general harder to comprehend than the most basic React pattern available: composition.) You don't use a cannon to kill a fly. I double checked with the Team to make sure I'm not missing something here, but they, too don't see a problem with the structure here. If you are still concerned please schedule a call before the retro or after the planning meeting tomorrow and I'll be happy to discuss it. |
||
import { Request as HarRequest } from 'har-format'; | ||
import * as React from 'react'; | ||
|
||
import { RequestSamples } from '../RequestSamples'; | ||
import { TryIt, TryItProps } from './TryIt'; | ||
|
||
type TryItWithRequestSamplesProps = Omit<TryItProps, 'onRequestChange'>; | ||
|
||
export const TryItWithRequestSamples: React.FC<TryItWithRequestSamplesProps> = props => { | ||
const [requestData, setRequestData] = React.useState<HarRequest | undefined>(); | ||
|
||
return ( | ||
<div> | ||
<Box mb={3}> | ||
<TryIt {...props} onRequestChange={setRequestData} /> | ||
</Box> | ||
{requestData && ( | ||
<Box mb={3}> | ||
<RequestSamples request={requestData} /> | ||
</Box> | ||
)} | ||
</div> | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,3 @@ | ||
export * from './TryIt'; | ||
export * from './TryItContainer'; | ||
export * from './TryItWithRequestSamples'; |
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.
Maybe we should test it?
To make sure it is being called with request object of proper shape?
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.
Definitely, maybe not exactly the shape - that's guaranteed by TypeScript - but also the contents, and whether it's called after an update.
And also the integration could be tested, whether StoplightProject / etc display RequestSamples at all.
I'll work on adding them.
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.
Yeah, when I wrote "shape" I meant more contents etc. ;)