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

Introduce 'Copy as a curl command' button to a debug request window #1747

Merged
merged 14 commits into from May 16, 2019

Conversation

Projects
None yet
4 participants
@wooyeong
Copy link
Contributor

commented Apr 28, 2019

Motivation:

  • Rarely, I want to copy a debug request to a curl command to put
    additional headers to the request. Even if I can copy it from the dev
    tools provided by browsers, opening them is annoying.

Modifications:

  • Introduced a 'Copy as a curl command' button
    • It copies the converted command to the clipboard, but not intuitive...
  • Added some methods to support curl export in Transport class
    • Each three transports has its own MIME type and body format to make
      a request in text

Result:

For your reference,

@wooyeong wooyeong requested review from hyangtack, minwoox and trustin as code owners Apr 28, 2019

Introduce 'export-as-curl' button to a debug request window
Motivation:

- Rarely, I want to copy a debug request to a curl command to put
additional headers to the request. Even if I can copy it from the dev
tools provided by browsers, opening them is annoying.

Modifications:

- Introduced a 'export-as-curl' button
  - It copys the converted command to the clipboard, but not intuitive...
- Added some methods to support curl export in `Transport` class
  - Each three transports has its own MIME type and body format to make
  a request in text

Result:

- Close #196

@wooyeong wooyeong force-pushed the wooyeong:export-as-curl branch from 6d4a5de to 0290cb4 Apr 28, 2019

@trustin

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

This is pretty cool! Just a few comments:

  • How about renaming the button to Copy as a cURL command and making it copy directly to the clipboard instead of showing anything in the right side pane?
    • When the command is copied to clipboard, we could show some notification using a Snackbar.
  • Could you handle the case where the request body contains '? You could use heredoc as explained in: https://stackoverflow.com/a/23930212
@wooyeong

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

Okay, I'll change the label and copy it directly w/o touching the panel on the right, and will attempt to use of Snackbar (actually I thought about it yesterday). Also, will take a look at the body part you mentioned as well.

By the way, is cURL right? according to the official repository and website, I think they usually write it down as curl or Curl nowadays?

@trustin

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

By the way, is cURL right?

Let's use the official notation, then. :-)

@trustin trustin added the new-feature label Apr 29, 2019

@wooyeong
Copy link
Contributor Author

left a comment

When both URI and body have '.

@minwoox
Copy link
Member

left a comment

Thanks a lot for this!
Just left a few comments.
Could you also fix the checkstyle failure?

Show resolved Hide resolved docs-client/src/containers/MethodPage/DebugPage.tsx Outdated
Show resolved Hide resolved docs-client/src/containers/MethodPage/DebugPage.tsx
Show resolved Hide resolved docs-client/src/containers/MethodPage/DebugPage.tsx Outdated
Show resolved Hide resolved docs-client/src/containers/MethodPage/DebugPage.tsx Outdated
Show resolved Hide resolved docs-client/src/lib/transports/transport.ts Outdated
Show resolved Hide resolved docs-client/src/lib/transports/transport.ts Outdated
@wooyeong

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

I worry about possible regressions when I apply checkstyle. Could you kindly take a look please?

@codecov

This comment has been minimized.

Copy link

commented May 2, 2019

Codecov Report

Merging #1747 into master will decrease coverage by 1.17%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1747      +/-   ##
============================================
- Coverage     72.93%   71.75%   -1.18%     
- Complexity     8153     8429     +276     
============================================
  Files           736      761      +25     
  Lines         32476    33871    +1395     
  Branches       3991     4168     +177     
============================================
+ Hits          23686    24305     +619     
- Misses         6758     7448     +690     
- Partials       2032     2118      +86
Impacted Files Coverage Δ Complexity Δ
...ringframework/boot/minimal/HelloConfiguration.java 0% <0%> (-100%) 0% <0%> (-3%)
...e/springframework/boot/tomcat/HelloController.java 0% <0%> (-100%) 0% <0%> (-3%)
...eria/client/encoding/GzipStreamDecoderFactory.java 0% <0%> (-100%) 0% <0%> (-3%)
...a/client/encoding/DeflateStreamDecoderFactory.java 0% <0%> (-100%) 0% <0%> (-3%)
...pringframework/boot/tomcat/HelloConfiguration.java 0% <0%> (-100%) 0% <0%> (-6%)
...ia/client/limit/ConcurrencyLimitingHttpClient.java 0% <0%> (-100%) 0% <0%> (-7%)
...rp/armeria/common/HttpHeadersJsonDeserializer.java 0% <0%> (-92%) 0% <0%> (-8%)
...rp/armeria/client/encoding/HttpDecodingClient.java 0% <0%> (-86.67%) 0% <0%> (-5%)
...rmeria/client/limit/ConcurrencyLimitingClient.java 0% <0%> (-85.72%) 0% <0%> (-16%)
...ework/boot/minimal/ValidationExceptionHandler.java 0% <0%> (-80.77%) 0% <0%> (-2%)
... and 201 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 318fd00...553fcfb. Read the comment docs.

@trustin
Copy link
Member

left a comment

You did a really good job given that it's your first time front-end work. 👍


private static copyTextToClipboard(text: string) {
const textArea = document.createElement('textarea');
textArea.style.opacity = '0.0';

This comment has been minimized.

Copy link
@trustin

trustin May 8, 2019

Member

Not an expert on this topic, but could this be display = 'none'?

This comment has been minimized.

Copy link
@wooyeong

wooyeong May 12, 2019

Author Contributor

I've tried display = 'none' and visibility = 'hidden', neither works well on chrome.
I guess dom's physical existence and visibility affect the behavior, but also I'm not an expert :)


const header = Object.keys(headers)
.map((title) => {
return `-H '${title}:${headers[title]}'`;

This comment has been minimized.

Copy link
@trustin

trustin May 8, 2019

Member

I'm curious it will look prettier if we insert a space after a colon (:).

This comment has been minimized.

Copy link
@wooyeong

wooyeong May 12, 2019

Author Contributor

Having a space makes it look better.

headers[docServiceDebug] = 'true';
}

const header = Object.keys(headers)

This comment has been minimized.

Copy link
@trustin

trustin May 8, 2019

Member

headerOpts or headerOptions ?

This comment has been minimized.

Copy link
@wooyeong

wooyeong May 12, 2019

Author Contributor

Changed to headerOptions.

@minwoox
Copy link
Member

left a comment

Just nit comments :)

}

protected getCurlPath(endpoint: Endpoint): string {
return endpoint.pathMapping.substring('exact:'.length);

This comment has been minimized.

Copy link
@minwoox

minwoox May 9, 2019

Member

How about removing this method and just handling it in DebugPage?
I think this can cause misunderstanding to the reader because this only handles an exact path mapping.

This comment has been minimized.

Copy link
@wooyeong

wooyeong May 12, 2019

Author Contributor

Removed getCurlPath, and I think it becomes simpler.

private onExport = () => {
const escapeSingleQuote = (text: string) => text.replace(/'/g, `'\\''`);

const endpointPath = this.state.endpointPath; // annotated service only

This comment has been minimized.

Copy link
@minwoox

minwoox May 9, 2019

Member

This line can be removed and the endpointPath can be declared where it needs (line 330).

This comment has been minimized.

Copy link
@wooyeong

wooyeong May 12, 2019

Author Contributor

Thanks :)


const endpointPath = this.state.endpointPath; // annotated service only
const additionalHeaders = this.state.additionalHeaders;
const queries = this.state.additionalQueries;

This comment has been minimized.

Copy link
@minwoox

minwoox May 9, 2019

Member

ditto

protected getCurlBody(
body: string,
_endpoint: Endpoint,
_method: Method,

This comment has been minimized.

Copy link
@minwoox

minwoox May 9, 2019

Member

nit: Could remove underscores before the parameters?

This comment has been minimized.

Copy link
@wooyeong

wooyeong May 12, 2019

Author Contributor

Since they are unused method parameters (an overridden method uses), they need to be started w/ an underscore.

This comment has been minimized.

Copy link
@minwoox

minwoox May 13, 2019

Member

Forgot that ESLInt has that rule. 😆

}

const header = Object.keys(headers)
.map((title) => {

This comment has been minimized.

Copy link
@hyangtack

hyangtack May 10, 2019

Contributor

nit: How about renaming title to name for better readability?

This comment has been minimized.

Copy link
@wooyeong

wooyeong May 12, 2019

Author Contributor

Changed to name.

@minwoox
Copy link
Member

left a comment

Just one nit. Great job!

if (this.props.exactPathMapping) {
const queries = this.state.additionalQueries;
uri =
`'${host}${escapeSingleQuote(path)}` +

This comment has been minimized.

Copy link
@minwoox

minwoox May 13, 2019

Member

Should remove exact: from the path.

This comment has been minimized.

Copy link
@wooyeong

wooyeong May 14, 2019

Author Contributor

Thanks. 😭

@minwoox
Copy link
Member

left a comment

Great job @wooyeong!
Thanks a lot for your hard work!

@minwoox

This comment has been minimized.

Copy link
Member

commented May 14, 2019

Please, fix the check style error. :-)

@wooyeong

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

I should be familiar w/ ' when I write code in typescript 😅. Sorry!

@minwoox minwoox added this to the 0.86.0 milestone May 15, 2019

@hyangtack

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

Nice work, @wooyeong ! Thanks for your patience.

@hyangtack hyangtack changed the title Introduce 'export-as-curl' button to a debug request window Introduce 'Copy as a curl command' button to a debug request window May 16, 2019

@hyangtack hyangtack merged commit a54c908 into line:master May 16, 2019

4 of 5 checks passed

codecov/project 71.75% (-1.18%) compared to 318fd00
Details
Travis CI - Pull Request Build Passed
Details
codecov/patch Coverage not affected when comparing 318fd00...553fcfb
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla Contributor License Agreement is signed.
Details
@hyangtack

This comment has been minimized.

Copy link
Contributor

commented May 16, 2019

Thank you for your first contribution, @wooyeong ! 👍

@wooyeong wooyeong deleted the wooyeong:export-as-curl branch May 16, 2019

@wooyeong

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

Thank you for reviewing this 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.