Skip to content
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

Show gRPCs in requests dialog #4127

Merged

Conversation

vincendep
Copy link
Contributor

@vincendep vincendep commented Oct 16, 2021

changelog(Improvements): gRPC requests will now appear in the request switcher

Closes #3271

@vincendep vincendep changed the title Show gRPC requests in RequestSwitcherModal Show gRPCs in requests dialog Oct 16, 2021
Copy link
Contributor

@develohpanda develohpanda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! We do need to add some UI for GRPC requests in the request switcher as well. For instance, the following is what I see when hitting cmd + p.

Ideally we follow a similar pattern to REST requests.

image

I also noticed the following lines: https://github.com/vincendep/insomnia/blob/21047c818165a63c79d9484edf723e39cd85416b/packages/insomnia-app/app/ui/components/modals/request-switcher-modal.tsx#L34-L35

We'll need to remove this (now that GRPC requests have been introduced to the request switcher), and determine whether there are any implications. I couldn't see any while browsing this file.

Lastly, could you also please (at the very least) add screenshots or gifs to PR descriptions? It really helps with code reviews. Thank you! 🙏🏽

@vincendep
Copy link
Contributor Author

Hi @develohpanda, so sorry for my lazyness 😄, I'll be more explicative for my next pull requests. Speaking of UI changes, do you think would be ok to show only "gRPC" (as Add Request dropdown),

Schermata 2021-10-18 alle 10 44 58

or maybe we should include the selected gRPC method some how?

@develohpanda
Copy link
Contributor

No worries at all! It would be nice to show the url + selected grpc method name. Should be fairly straightforward to get that information I think

@filfreire
Copy link
Member

@vincendep, in case you are interested you could achieve what @develohpanda mentioned with something like:

Near line 224:

       finalUrl = joinUrlAndQueryString(finalUrl, buildQueryStringFromParams(request.parameters));
     }
 
+    if (isGrpcRequest(request)) {
+      finalUrl = request.url + request.protoMethodName;
+    }
+
     const match = fuzzyMatchAll(
       searchStrings,
       [request.name, finalUrl, request.method || '', this._groupOf(request).join('/')],

Near line 477:

                       </div>
                       <div className="margin-left-xs faint">
                         { isRequest(r) ? <MethodTag method={r.method} /> : null }
-                        <Highlight search={searchString} text={r.url} />
+                        <Highlight
+                          search={searchString}
+                          text={ isGrpcRequest(r) ? r.url + r.protoMethodName : r.url }
+                        />
                       </div>
                     </Button>
                   </li>

Example of how it would look like:

Screenshot 2021-12-13 at 12 06 53

Screenshot 2021-12-13 at 12 02 32

@vincendep
Copy link
Contributor Author

Sorry about end of communications by I didn't found time to work on this.
Hi @filfreire, thanks for your help, I'll give it a try soon and see if I can close this. Btw I think that would be nice to show also the gRPC tag in place of the method tag

@vercel vercel bot temporarily deployed to Preview December 16, 2021 15:56 Inactive
@vincendep
Copy link
Contributor Author

Here a screenshoot of the update

Schermata 2021-12-18 alle 10 06 01

@vercel vercel bot temporarily deployed to Preview December 23, 2021 13:30 Inactive
Copy link
Member

@filfreire filfreire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, great work @vincendep 👍

Screenshot 2022-01-04 at 17 32 35

@filfreire
Copy link
Member

FYI @dimitropoulos @develohpanda this PR would be a good candidate to get merged before the next release.

@filfreire filfreire added T-healthy Change was tested and seems ok T-tested-by-author Change has been tested by Insomnia Team / Kong Test Engineers labels Jan 12, 2022
@vercel vercel bot temporarily deployed to Preview January 13, 2022 04:47 Inactive
Copy link
Contributor

@develohpanda develohpanda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates @vincendep! I pushed a few small commits, please take a look!

These commits remove a redundant comment and improves some TS types that were incorrect after the changes in this PR (due to a quirk of TS and not your fault 🤗)


// OPTIMIZATION: This only filters if we have a filter
let matchedRequests = workspaceChildren
.filter(isRequest)
.filter(child => isRequest(child) || isGrpcRequest(child))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately with this change we lose type safety (matchedRequest should not be of type RequestGroup). There's a known bug in TS regarding this, and I haven't seen any reliable workarounds for it.

After
image

Before
image

I've updated some of the type definitions here to bring the as cast alongside: e348696

Copy link
Contributor Author

@vincendep vincendep Jan 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @develohpanda. Thanks for the explanation, given thath I'm a beginner with TS every advice is welcome

@vercel vercel bot temporarily deployed to Preview January 14, 2022 18:56 Inactive
@dimitropoulos dimitropoulos merged commit ba15a73 into Kong:develop Jan 14, 2022
@vincendep vincendep deleted the fix/show-grpc-request-in-navigation branch June 23, 2022 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-healthy Change was tested and seems ok T-tested-by-author Change has been tested by Insomnia Team / Kong Test Engineers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gRPC requests do not appear in request navigation (Quick Switch, Recent)
4 participants