Correctly initialise gRPCurl text area

[gerektoolhy]

Description

#241 introduced feature where a grpcurl command would be rendered. The grpcurl could then be copied and run in terminal repeatedly.

Issue

After some use, I have noticed a small bug where on initial page load, the grpcurl would contain an incorrectly serialized payload [Object object]. Expectation is that the payload should contain a valid JSON data. At a minimum this has to be an empty JSON object {}, or anything that has been entered either from the first tab, or manually in the Request box.

Root cause

The issue is introduced by line https://github.com/fullstorydev/grpcui/blob/master/internal/resources/webform/webform.js#L158 which calls, which calls updateCurlCommand(requestObj);. The problem with this line is that requestObj is not a string parameter. This object ends up being concatenated to a string, and gets translated to Object object.

To trace this issue down I have instrumented updateCurlCommand() function and logged out how many times it was called and the stack trace along with it. It turned out that on page load this function was called twice. Surprisingly, the first call contained the correct JSON value, but the second not.

The first call was made from updateJsonRequest, which converted request object to string and passed that to updateCurlCommand.

Fix

It turns out, that the second call to updateCurlCommand is not needed, and thus can be removed.

After fix the grpcurl textbox correctly contains a valid JSON as data parameter:

grpcurl -plaintext -d '{
  "diffIds": []
}' localhost:9011 <host_redacted>

[gerektoolhy]

FYI @dragonsinth

[TilGP]

Was able to replicate the issue and verify that the fix works

[gerektoolhy]

@TilGP anything else needs for this to be merged?

[TilGP]

@TilGP anything else needs for this to be merged?

Apparently a review from someone with write access, which I am not. (I'm not affiliated with fullstorydev either). I just reviewed it with the hope, that it will bring some attention from the maintainers to this PR.

[gerektoolhy]

@TilGP got it, I wrongly assumed that you were. Thanks for confirming the fix though.

FYI @dragonsinth @jhump

[jhump]

LGTM. Though I also do not have write access to this repo 😂. (I left FullStory a couple of years ago.)

@dragonsinth, @gpassini, could one of you two take a look?

[dragonsinth]

I'm not sure this is the best fix. I think the underlying problem is that updateJSONRequest takes an object/value while updateCurlCommand wants a string. I'm not even sure we want to use the same format... updateJSONRequest internally uses JSON.stringify(req, null, 2) while I think updateCurlCommand should internally use plain JSON.stringify(req).

[dragonsinth]

this part actually seems fine since updateJSONRequest calls this internally

[gerektoolhy]

@dragonsinth can you clarify please. The call to updateCurlCommand is not required here, since updateJSONRequest will do that. So removing this call actually fixes the issue.

[gerektoolhy]

@dragonsinth anything else to address in this PR?

[gerektoolhy]

I'm not sure this is the best fix. I think the underlying problem is that updateJSONRequest takes an object/value while updateCurlCommand wants a string. I'm not even sure we want to use the same format... updateJSONRequest internally uses JSON.stringify(req, null, 2) while I think updateCurlCommand should internally use plain JSON.stringify(req).

updateJSONRequest is calling updateCurlCommand, so there is no need first call updateJSONRequest. Removing the call to updateCurlCommand fixes the issue.

[dragonsinth]

I'm not sure this is the best fix. I think the underlying problem is that updateJSONRequest takes an object/value while updateCurlCommand wants a string. I'm not even sure we want to use the same format... updateJSONRequest internally uses JSON.stringify(req, null, 2) while I think updateCurlCommand should internally use plain JSON.stringify(req).

@gerektoolhy this is still a problem ^^

The issue is that the JSON string we shove into jsonRawTextArea should be pretty-printed on multiple lines, but the JSON string we shove into gRPCurlTextArea inside of updateCurlCommand needs to be single-line compact.

[dragonsinth]

@gerektoolhy I pushed a commit onto your branch to try to address.. but I'll need you to retest with my changes and make sure they are good.

[gerektoolhy]

It does fix the issue. However, this now works in a different way than before. The data parameter for gRPCurl is now all contained in a single line, instead of being pretty-printed. Screenshot:

The issue is that the JSON string we shove into jsonRawTextArea should be pretty-printed on multiple lines, but the JSON string we shove into gRPCurlTextArea inside of updateCurlCommand needs to be single-line compact.

So the reason the initial implementation used pretty printing was because the premise is that people will want to read the payload of the grpcurl, and even modify it. If the payload is contained on a single line, it becomes hard to do that.

The grpcurl command works in both formats, either as a single line, or multi-line. I have just tested this again to make sure that is the case. Of course, the multi-line is my personal preferred option. And i would argue, that having it in a human-readable format is desirable more often than the other way around.

[dragonsinth]

Some quick thoughts... it seems to me in most cases the single-line input for a command line is a lot more portable? If you want to edit the request, you can edit it via the UI or jsonRawTextArea, and if you really want the multiline version, you could just grab it from jsonRawTextArea, right?

[gerektoolhy]

@dragonsinth I guess it can be argued both ways which format is more usable. From my personal perspective, my use case is that I generate a grpcurl command via grpcui, with some initial data. Then I work in the terminal, continuously resending the request, and sometimes modifying parameters. I could go back to UI and update the request and then copy the grpcurl command, thats true, but that is more clicks. However, that is just me, and there are many more different use cases.

Trying to think of another alternative... What about specifying a command line argument which would configure the formatting of the grpcurl command that is generated? This could look like this:

# Option 1
grpcui --pretty-print -plaintext localhost:9009

# Option 2
grpcui --grpcurl-format pretty -plaintext localhost:9009
grpcui --grpcurl-format oneline -plaintext localhost:9009

I am up for implement this. The default could be one-line format, and this could be changed to pretty-print using the param.

[TilGP]

What about specifying a command line argument which would configure the formatting of the grpcurl command that is generated?

I am on your side with this, I would much prefer a human-readable format, while others may prefer a compact format.
So a CLI-Argument is a good comprise.
It would be nice to have a config file to set this permanently.