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

Use monaco editor for Request Body in Query Editor #4516

Merged
merged 15 commits into from
Mar 31, 2023

Conversation

Dogacel
Copy link
Contributor

@Dogacel Dogacel commented Nov 3, 2022

Motivation:

Update the debug form to use Microsoft's well-known Monaco Editor.

Modifications:

  • Integrate Monaco editor to be used in the request body text editor.
    • Easy-to-use editor with bracket highlight, auto-close tags, etc.
    • Enable JSON syntax highlighting
    • Show JSON syntax errors (For future, we will generate JSON schemas for protos to verify the object syntax as well)

Result:

Screen.Recording.2022-11-10.at.22.44.28.mov

The example used is GrpcDocServiceTest the method is UnaryMethod.
This PR is required to enable auto-complete:

@Dogacel Dogacel changed the title use monaco editor Use monaco editor for Request Body in Query Editor Nov 3, 2022
docs-client/package.json Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Nov 4, 2022

Codecov Report

❗ No coverage uploaded for pull request base (main@f769310). Click here to learn what that means.
Patch has no changes to coverable lines.

❗ Current head 0bc19f7 differs from pull request most recent head 4ab898e. Consider uploading reports for the commit 4ab898e to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4516   +/-   ##
=======================================
  Coverage        ?   74.08%           
  Complexity      ?    18186           
=======================================
  Files           ?     1537           
  Lines           ?    67469           
  Branches        ?     8537           
=======================================
  Hits            ?    49987           
  Misses          ?    13412           
  Partials        ?     4070           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Dogacel Dogacel mentioned this pull request Dec 31, 2022
ikhoon pushed a commit to line/centraldogma that referenced this pull request Jan 2, 2023
## Motivation
Allow viewing/editing files.

## Modification
- Add monaco editor. https://www.npmjs.com/package/@monaco-editor/react
- Update FileList to allow recursive structure for folder.
- Add file content.
- Add some placeholder components.
- Add edit and preview modes

### Notes regarding the choice of file editor  
The old app is using Ace. It's quite stable but there are some warnings and it needs some workaround to make it work with Nextjs. https://github.com/securingsincity/react-ace

Thus, we look into other options https://en.wikipedia.org/wiki/Comparison_of_JavaScript-based_source_code_editors.
Both Codemirror and Monaco seem to be a great choice in terms of setup and UI. There's a caveat when using Monaco for JSON. Need a workaround to format the code properly.

Since Armeria is moving away from Codemirror line/armeria#4516, we go with Monaco. 

## Result
<img width="1253" alt="Screen Shot 2022-12-26 at 18 04 18" src="https://user-images.githubusercontent.com/5079588/209541874-48470cc4-fe25-4c63-9136-d7a977ef9123.png">

<img width="829" alt="Screen Shot 2022-12-27 at 11 52 15" src="https://user-images.githubusercontent.com/5079588/209612997-af4d8506-edb9-4767-95a0-99c863d5c20e.png">
@Dogacel
Copy link
Contributor Author

Dogacel commented Jan 2, 2023

Waiting until

is merged so we can have the full autocomplete feature right away.

@Dogacel
Copy link
Contributor Author

Dogacel commented Jan 31, 2023

Updated with the latest master ^

@Dogacel
Copy link
Contributor Author

Dogacel commented Feb 8, 2023

@ikhoon Can we release this in 1.22.0 ? This is ready to be tested and hopefully it will be useful.

@ikhoon ikhoon added this to the 1.23.0 milestone Feb 9, 2023
@ikhoon
Copy link
Contributor

ikhoon commented Feb 9, 2023

@ikhoon Can we release this in 1.22.0 ? This is ready to be tested and hopefully it will be useful.

Thanks for your hard work. 🙇‍♂️ This is so a nice feature that users should love. That being said, we didn't have the time to review this PR. There are some features needed to release in a timely manner for LINE internal users.

Let me review this after releasing 1.22.0 and ship it in 1.23.0.

@Dogacel
Copy link
Contributor Author

Dogacel commented Feb 9, 2023

@ikhoon Can we release this in 1.22.0 ? This is ready to be tested and hopefully it will be useful.

Thanks for your hard work. 🙇‍♂️ This is so a nice feature that users should love. That being said, we didn't have the time to review this PR. There are some features needed to release in a timely manner for LINE internal users.

Let me review this after releasing 1.22.0 and ship it in 1.23.0.

Sure, thanks for the heads-up 😉

@Dogacel
Copy link
Contributor Author

Dogacel commented Feb 22, 2023

bump

@jrhee17
Copy link
Contributor

jrhee17 commented Feb 22, 2023

Sorry 😅 We definitely didn't forget about this PR (it's going to be one of the highlights of the next release)
Let me take a look tomorrow morning

@ikhoon
Copy link
Contributor

ikhoon commented Feb 23, 2023

I found a bug that a JSON schema for AnnotatedDocServiceTest$MyService.json() isn't correctly created.

public String json(JsonRequest request) {
return request.bar;
}

We may apply useParameterAsRoot to the JSON request.

Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

This is so awesome for gRPC and Thrift.
However, we need to consider exceptional rules for annotated services.

docs-client/src/containers/App/index.tsx Outdated Show resolved Hide resolved
docs-client/src/containers/MethodPage/RequestBody.tsx Outdated Show resolved Hide resolved
@ikhoon
Copy link
Contributor

ikhoon commented Feb 23, 2023

Had a chat with other team members and we decided to support gRPC and Thrift at the moment.
So please disable JSON schema for annotated services.
Annotated services will take advantage of JSON schema later when it is ready.

@Dogacel
Copy link
Contributor Author

Dogacel commented Feb 23, 2023

Had a chat with other team members and we decided to support gRPC and Thrift at the moment. So please disable JSON schema for annotated services. Annotated services will take advantage of JSON schema later when it is ready.

What is the easiest way to check whether a call is gRPC or thrift? Should I take a look at availableMimeTypes?

Maybe via this:

const isRPC = grpcUnframedTransport.supportsMimeType(mimeType)
 || thriftTransport.supportsMimeType(mimeType);

@ikhoon
Copy link
Contributor

ikhoon commented Feb 23, 2023

It looks like the easiest way. 😅

@Dogacel Dogacel force-pushed the dogac/use-monaco-editor branch from 7235e1f to 86fd7be Compare March 17, 2023 10:36
@Dogacel Dogacel requested a review from ikhoon March 17, 2023 12:57
Copy link
Member

@trustin trustin left a comment

Choose a reason for hiding this comment

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

Lovely. Thanks a lot, @Dogacel! 🙇

@Dogacel
Copy link
Contributor Author

Dogacel commented Mar 24, 2023

I am blocked by this:

And I suppose my package-lock.json file is not correct. What is the recommended node/npm version I should use to update the package-lock? Is it possible that last lock file had issues?

@trustin
Copy link
Member

trustin commented Mar 28, 2023

And I suppose my package-lock.json file is not correct. What is the recommended node/npm version I should use to update the package-lock? Is it possible that last lock file had issues?

According to docs-client/build.gradle, we're using nodejs 16.14.0 and npm 8.5.2, which are probably outdated. Let me push a commit to this PR to update them to the newer versions and resolve the conflicts.

@trustin
Copy link
Member

trustin commented Mar 28, 2023

@Dogacel It seems like the npm and webpack don't complain anymore at least. Could you check if my fix works as intended?

@trustin
Copy link
Member

trustin commented Mar 28, 2023

I had to downgrade nodejs from 18 to 16 because our CI machines are using old GLIBC, but it still seems to work.

- Do not set the language option to 'json' if no JSON schema is available.
Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

Thanks a ton, @Dogacel! ❤️
image

@@ -77,6 +80,19 @@ const GraphqlRequestBody: React.FunctionComponent<Props> = ({
const [variablesText, setVariablesText] = useState('');
const [schema, setSchema] = useState<GraphQLSchema | undefined>();

const monacoEditor = useMonaco();

useEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Recently, I read an article about useEffect() https://react.dev/learn/you-might-not-need-an-effect
Based on the article, I thought this was not a good case of using useEffect. So I used useMemo() instead.
https://react.dev/learn/you-might-not-need-an-effect#caching-expensive-calculations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I got confused for a second because commit title was [- Replace useMemo with useEffect.] 😅 I think it makes sense, thanks for adding this 👍

Comment on lines -16 to -17
"codemirror": "^5.65.2",
"codemirror-graphql": "^1.2.17",
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@ikhoon
Copy link
Contributor

ikhoon commented Mar 29, 2023

I had to downgrade nodejs from 18 to 16 because our CI machines are using old GLIBC, but it still seems to work.

I will check this separetely.

@ikhoon
Copy link
Contributor

ikhoon commented Mar 29, 2023

I found out that incompatibility with NodeJS 18 and AWS EC2 is a known issue.
nvm-sh/nvm#2972
https://docs.aws.amazon.com/sdk-for-javascript/v2/developer-guide/setting-up-node-on-ec2-instance.html
It seems to be better to upgrade the NodeJS version when AWS supports it.

Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

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

The code changes itself look good to me! Also looking into the file issue mentioned here by @ikhoon #4516 (comment)

Comment on lines 137 to 140
excludedPackageTest: (packageName: string) => {
// @monaco-editor/react and monaco-editor are the same package and causes license duplication.
return packageName.includes("monaco-editor");
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@trustin @ikhoon @jrhee17

As a workaround we can add a separate file web-licenses-extra.txt and copy paste licenses from the monaco-editor packages to unblock ourselves. Wdy think?

Copy link
Contributor

@jrhee17 jrhee17 Mar 30, 2023

Choose a reason for hiding this comment

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

Did you try running with perChunkOutput:false by any chance? I don't think we want to generate license files per chunk anyways and it seemed to run without errors in my local env.

ref: https://github.com/xz64/license-webpack-plugin/blob/master/DOCUMENTATION.md

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That + @ikhoon 's changes regarding the number of chunks actually fixed the error and now we have a lot more licenses inside the license file. Probably it was what it should have been in the first place.

@Dogacel
Copy link
Contributor Author

Dogacel commented Mar 30, 2023

I have tested this one last time and everything looks good to me. Thanks, everyone for all the collaborative effort. It included lots of learning experiences for me, I hope people will enjoy using doc service more now 😄

Copy link
Contributor

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

Great job, @Dogacel! 👍 👍 👍

@minwoox minwoox merged commit 8652e5e into line:main Mar 31, 2023
@minwoox
Copy link
Contributor

minwoox commented Mar 31, 2023

This will be one of the key features of the year. 🎉 🎉 🎉
Thanks, @Dogacel and others for helping. 😄

ikhoon pushed a commit to ikhoon/centraldogma that referenced this pull request Apr 10, 2023
## Motivation
Allow viewing/editing files.

## Modification
- Add monaco editor. https://www.npmjs.com/package/@monaco-editor/react
- Update FileList to allow recursive structure for folder.
- Add file content.
- Add some placeholder components.
- Add edit and preview modes

### Notes regarding the choice of file editor  
The old app is using Ace. It's quite stable but there are some warnings and it needs some workaround to make it work with Nextjs. https://github.com/securingsincity/react-ace

Thus, we look into other options https://en.wikipedia.org/wiki/Comparison_of_JavaScript-based_source_code_editors.
Both Codemirror and Monaco seem to be a great choice in terms of setup and UI. There's a caveat when using Monaco for JSON. Need a workaround to format the code properly.

Since Armeria is moving away from Codemirror line/armeria#4516, we go with Monaco. 

## Result
<img width="1253" alt="Screen Shot 2022-12-26 at 18 04 18" src="https://user-images.githubusercontent.com/5079588/209541874-48470cc4-fe25-4c63-9136-d7a977ef9123.png">

<img width="829" alt="Screen Shot 2022-12-27 at 11 52 15" src="https://user-images.githubusercontent.com/5079588/209612997-af4d8506-edb9-4767-95a0-99c863d5c20e.png">
ikhoon pushed a commit to ikhoon/centraldogma that referenced this pull request Apr 20, 2023
## Motivation
Allow viewing/editing files.

## Modification
- Add monaco editor. https://www.npmjs.com/package/@monaco-editor/react
- Update FileList to allow recursive structure for folder.
- Add file content.
- Add some placeholder components.
- Add edit and preview modes

### Notes regarding the choice of file editor  
The old app is using Ace. It's quite stable but there are some warnings and it needs some workaround to make it work with Nextjs. https://github.com/securingsincity/react-ace

Thus, we look into other options https://en.wikipedia.org/wiki/Comparison_of_JavaScript-based_source_code_editors.
Both Codemirror and Monaco seem to be a great choice in terms of setup and UI. There's a caveat when using Monaco for JSON. Need a workaround to format the code properly.

Since Armeria is moving away from Codemirror line/armeria#4516, we go with Monaco. 

## Result
<img width="1253" alt="Screen Shot 2022-12-26 at 18 04 18" src="https://user-images.githubusercontent.com/5079588/209541874-48470cc4-fe25-4c63-9136-d7a977ef9123.png">

<img width="829" alt="Screen Shot 2022-12-27 at 11 52 15" src="https://user-images.githubusercontent.com/5079588/209612997-af4d8506-edb9-4767-95a0-99c863d5c20e.png">
@ikhoon
Copy link
Contributor

ikhoon commented Jun 7, 2023

I had to downgrade nodejs from 18 to 16 because our CI machines are using old GLIBC, but it still seems to work.

I've upgraded our self-hosted CI machines to the latest OS that will be compatible NodeJS 18.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants