feat: RequestSamples in docs #864

marcelltoth · 2021-02-12T15:28:57Z

Summary

Resolves #683

Not based on @mpodlasin's branch, I decided to take another approach.

Changes of plans

I made a change compared to what we discussed earlier: The solution is not using the IHttpRequest type for inter-component communication, but TryIt is now exposing a HAR directly. The reason is three factors combined:

IHttpRequest is a bit of pain to work with. It imposes some format choices that neither fetch nor HAR are using, leading to a need for a back-and-forth conversion.
It isn't a clearly defined format either, one of the most important pieces, body is left undefined. (It's generic and <T = any>.)
Probably because of the above, we are not using it anywhere else either.

So I decided that HAR is probably a better format even for passing data around, and it's a lot simpler this way because RequestSamples needs a HAR anyway.

TODO

There's some cleanup to do in TryIt, it grew large and ugly, and also I probably put a lot of repetitive code in there. I'm working on that still but any suggestions are welcome.

marcelltoth · 2021-02-12T15:30:02Z

packages/elements/src/components/TryIt/TryIt.tsx

    },
  ];
 }

+async function buildHarRequest({


This function requires a lot of cleanup, I know. And potentially a move to another file along with buildFetchRequest. (Which @mpodlasin did in his version as far as I remember.)

marcelltoth · 2021-02-12T15:33:04Z

packages/elements/src/components/TryIt/TryItContainer.tsx

@@ -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) => {


I moved it next to the rest of TryIt as

It's not a public component, the only place we use it is within StoplightProject, not even in platform. It's really a piece of StoplightProject, I was also considering creating a folder for it under components just like how we have API. Can do that if you prefer.

We discussed previously that this container / component distinction is non-ideal for many reasons. It's hard to find the file, to know which one you're talking / reading about, and in general, they are all just components.

netlify · 2021-02-12T15:33:19Z

Deploy preview for stoplight-elements ready!

Built with commit bf769a9

https://deploy-preview-864--stoplight-elements.netlify.app

philsturgeon · 2021-02-12T15:46:45Z

Parameters are showing up escaped.

marcelltoth · 2021-02-15T08:04:11Z

Parameters are showing up escaped.

Of course, that's because the default value for a path parameter is {pathParamName} which gets escaped to %pathParamName%7D. That's the same fallback behavior we have if you hit Send on such thing.

We can change the fallback if you'd like, but I think if we implement the Required property on params this will become a non-issue anyway.

mpodlasin

This looks really good!

mpodlasin · 2021-02-15T11:28:06Z

packages/elements/src/components/TryIt/TryIt.tsx

+   * 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;


Maybe we should test it?

To make sure it is being called with request object of proper shape?

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.

Yeah, when I wrote "shape" I meant more contents etc. ;)

packages/elements/src/components/TryIt/TryIt.tsx

philsturgeon · 2021-02-15T13:09:14Z

packages/elements/src/components/TryIt/TryItWithRequestSamples.tsx

@@ -0,0 +1,25 @@
+import { Box } from '@stoplight/mosaic';


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.

It's not inheritance, it's composition. And yes, that's what we'll end up with, and we'll likely call it TryItStack or something.
Just like how we call TocDocsAndTryItSideBySide a SidebarLayout. And a SidebarLayoutWithDataFetching is an API. That's how React works IMO.

My naming is non-ideal, that's a given. Feel free to suggest another.

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?

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 FooAndBarAndBaz you don't have to first build a FooAndBar.

bunging it into Try It just so it has access to some of the data try it is creating

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.

mmiask

Tests and stories good look to me! RequestSamples already has its own test, so this should be enough.

As for the code itself, I see that @mpodlasin already reviewed this so I didn't touch that part

mmiask · 2021-02-15T16:43:29Z

packages/elements/src/components/RequestSamples/RequestSamples.tsx

@@ -54,10 +56,11 @@ export const RequestSamples = React.memo<RequestSamplesProps>(({ request }) => {
      </Panel.Titlebar>
      <Panel.Content p={0}>
        <CodeViewer
+          aria-label={requestSample ?? fallbackText}


Does the label take the full string as a value?

Well it's not ideal, but that's how I thought we could provide a text-only alternative. (That's what label is.) Helpful for both testing and screen readers who may have a hard time reading the codeviewer. Ideally CodeViewer should handle this on its own, but this is an acceptable compromise IMO.

Marcell Toth added 3 commits February 12, 2021 15:07

feat: implement TryIt's onRequestChange

ae7babf

feat: create TryItWithRequestSamples and move TryItContainer

30c10a5

feat: use TryItWithRequestSamples everywhere

0e53184

marcelltoth self-assigned this Feb 12, 2021

marcelltoth requested a review from philsturgeon as a code owner February 12, 2021 15:28

marcelltoth requested a review from a team February 12, 2021 15:28

marcelltoth commented Feb 12, 2021

View reviewed changes

mpodlasin approved these changes Feb 15, 2021

View reviewed changes

mpodlasin reviewed Feb 15, 2021

View reviewed changes

Marcell Toth added 2 commits February 15, 2021 13:34

refactor: extract useTextRequestBodyState

e1cd84c

refactor: extract build-request.ts

79e9d56

philsturgeon reviewed Feb 15, 2021

View reviewed changes

marcelltoth marked this pull request as draft February 15, 2021 14:31

test: add tests and stories for TryItWithRequestSamples

bf769a9

marcelltoth marked this pull request as ready for review February 15, 2021 16:31

mmiask approved these changes Feb 15, 2021

View reviewed changes

marcelltoth merged commit 17232d7 into v7 Feb 15, 2021

marcelltoth deleted the feat/request-samples-in-docs branch February 15, 2021 18:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: RequestSamples in docs #864

feat: RequestSamples in docs #864

marcelltoth commented Feb 12, 2021

marcelltoth Feb 12, 2021

marcelltoth Feb 12, 2021

netlify bot commented Feb 12, 2021 •

edited

Loading

philsturgeon commented Feb 12, 2021

marcelltoth commented Feb 15, 2021

mpodlasin left a comment

mpodlasin Feb 15, 2021

marcelltoth Feb 15, 2021

mpodlasin Feb 15, 2021

philsturgeon Feb 15, 2021

marcelltoth Feb 15, 2021

philsturgeon Feb 15, 2021

marcelltoth Feb 15, 2021

mmiask left a comment

mmiask Feb 15, 2021

marcelltoth Feb 15, 2021

feat: RequestSamples in docs #864

feat: RequestSamples in docs #864

Conversation

marcelltoth commented Feb 12, 2021

Summary

Changes of plans

TODO

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netlify bot commented Feb 12, 2021 • edited Loading

philsturgeon commented Feb 12, 2021

marcelltoth commented Feb 15, 2021

mpodlasin 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

mmiask left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netlify bot commented Feb 12, 2021 •

edited

Loading