Replace SyntaxHighlighter with Monaco Editor. #9728

nojaf · 2024-03-21T14:46:20Z

Description

This replaces react-syntax-highlighter with @monaco-editor/react.

Motivation and Context

The key motivation for us is to see large response bodies all the time.

How Has This Been Tested?

I've been playing with this manually on my local machine.
The endpoint I'm testing is retrieving 2.8 MB of JSON and it gets instantly rendered in my browser.

Screenshots (if appropriate):

Checklist

My PR contains...

No code changes (src/ is unmodified: changes to documentation, CI, metadata, etc.)
Dependency changes (any modification to dependencies in package.json)
[?] Bug fixes (non-breaking change which fixes an issue)
Improvements (misc. changes to existing features)
Features (non-breaking change which adds functionality)

My changes...

are breaking changes to a public API (config options, System API, major UI change, etc).

I'm not quite sure yet what to do with the syntaxHighlight.theme setting. These themes were tied to SyntaxHighlighter and the Monaco editor has different themes out of the box.
Please let know how we can deal with this.

are breaking changes to a private API (Redux, component props, utility functions, etc.).
are breaking changes to a developer API (npm script behavior changes, new dev system dependencies, etc).
are not breaking changes.

Documentation

My changes do not require a change to the project documentation.
My changes require a change to the project documentation.

I'm not sure here: please tell me.

If yes to above: I have updated the documentation accordingly.

Automated tests

My changes can not or do not need to be tested.
My changes can and should be tested by unit and/or integration tests.
If yes to above: I have added tests to cover my changes.
If yes to above: I have taken care to cover edge cases in my tests.
All new and existing tests passed.

As I'm new to this, I'm not quite sure what needs to be done here. Again, let me know and I'll look into it.

src/core/syntax-highlighting.js

test/unit/components/highlight-code.jsx

heldersepu · 2024-03-21T18:28:46Z

https://raw.githack.com/nojaf/swagger-ui/large-bodies/dist/index.html#/pet/findPetsByStatus
I tested your version and I'm not getting the same as your image...

the <div class="highlight-code"> seem to be just a few pixels of height

heldersepu · 2024-03-23T23:17:59Z

https://raw.githack.com/nojaf/swagger-ui/large-bodies/dist/index.html#/pet/findPetsByStatus I tested your version and I'm not getting the same as your image...

the <div class="highlight-code"> seem to be just a few pixels of height

@nojaf I think the issue here might be just an old bundle, I added my changes to my branch and the height is fine:
https://raw.githack.com/heldersepu/swagger-ui/test/dist/index.html?docExpansion=None&defaultModelsExpandDepth=None#/pet/findPetsByStatus

heldersepu · 2024-03-23T23:48:55Z

I changed the theme to be dark and disabled the minimap to get as close as possible as it was:

simple changes:
45c7631

nojaf · 2024-03-25T08:14:10Z

Hi @heldersepu, thank you for the feedback!
I believe I addressed all your remarks.

Any thoughts on what to do with the syntaxHighlight.theme setting?

And could I get the workflow approval?

heldersepu · 2024-03-25T14:05:57Z

@nojaf rebuild the bundles when you can I still can see the old version:
https://raw.githack.com/nojaf/swagger-ui/large-bodies/dist/index.html#/pet/findPetsByStatus

would be nice to integrate the syntaxHighlight.theme into your changes

I'm not in control of the workflow some one from the team should do that

heldersepu · 2024-03-25T14:07:53Z

@glowcloud & @char0n 👀

nojaf · 2024-04-03T07:26:25Z

Hi @heldersepu, apologies for the delay. I took some time off.
I've rebuilt all bundles.

would be nice to integrate the syntaxHighlight.theme into your changes

Happy to do this, but could use a pointer. Do we care about the existing values?

heldersepu · 2024-04-03T16:00:24Z

I don't think the existing theme values are compatible with the Monaco editor
test see what happens if an invalid theme is given, maybe there is a way we can provide a default or validate is one of the allowed themes

nojaf · 2024-04-04T13:47:51Z

So, some e2e tests are failing like:

swagger-ui/test/e2e-cypress/e2e/bugs/4943.cy.js

Lines 10 to 19 in 3e81a4f

    
           it("should render oneOf property correctly", () => { 
        
             cy 
        
               .visit("/?url=/documents/bugs/4943.yaml") 
        
               .get("#operations-Test-postTest") 
        
               .click() 
        
               .get(".try-out__btn") 
        
               .click() 
        
               .get(".microlight") 
        
               .contains("<c>\n\t</c>") 
        
           })

The problem presumably is in the assertion and might not be an actual problem.

To run this: npm run cy:start and in a different terminal npm run cy:run -- --spec "**/4943.cy.js".

Could I please get some pointers on how to address this?
I have never seen this Cypress thing up close and it is a tad overwhelming.

nojaf · 2024-04-04T15:53:46Z

I don't think the existing theme values are compatible with the Monaco editor

They are not, I've updated the setting to work with the Monaco themes.

@glowcloud & @char0n what are your thoughts on this PR, please?
I'm trying to get a sense of what my chances of getting this merged are.

heldersepu · 2024-04-05T12:53:57Z

Nice job @nojaf !
regarding the e2e test failing look at the history of the PR that added that see if this is not accidentally reintroducing that bug

nojaf · 2024-04-10T10:11:53Z

Hey everyone 👋,

With the recent changes in #9783, where the highlighting component got extracted, I've been inspired to pivot my approach a bit. I'm now considering focusing my efforts on developing a plugin, rather than continuing to push for my original PR. It's been a few weeks (three, to be exact), and I'm still not sure if there's any appetite for merging my PR.

For those who might come across this thread in the future and are looking to integrate Monaco into SwaggerUI, I've had some success doing so with a plugin approach. Here’s a quick snippet to show you how I managed to hook in Monaco:

import loader from "https://esm.sh/@monaco-editor/loader@1.4.0"

const nojafPlugin = {
  components: {
    SyntaxHighlighter: (props) => {
      const React = props.React;
      const myRef = React.createRef();
      React.useEffect(() => {
        let editor = null;

        const cancelable = loader.init();
        cancelable.then(monaco => {
          if (myRef.current) {
            editor = monaco.editor.create(myRef.current, {
              value: props.children,
              language: props.language
            })
          }
        }).catch(
          (error) =>
            error?.type !== 'cancelation' && console.error('Monaco initialization: error:', error),
        );

        return () => {
          if (cancelable && !myRef.current) {
            cancelable.cancel();
          }

          if (editor) {
            console.log(editor);
            editor.dispose();
          }
        }

      }, [myRef, props.children]);


      // The container needs a height.
      const maxHeight = 450;
      const newLineCount = !props.children ? 0 : (props.children.match(/\n/g)).length
      const proposedHeight = (newLineCount * 14)
      const minHeight = Math.max(100, Math.min(proposedHeight, maxHeight));

      return React.createElement("div", { ref: myRef, style: { minHeight: `${minHeight}px` } }, null);
    }
  }
}

And you can integrate it like so:

window.onload = function() {
  // Build a system
  const ui = SwaggerUIBundle({
    ...,
    plugins: [
      SwaggerUIBundle.plugins.DownloadUrl,
      nojafPlugin
    ],
  })

  window.ui = ui

  ui.initOAuth({...})
}

A few key takeaways from my experience:

Make sure your script tag includes type=module when using the Monaco loader import.
While this component does a decent job, be aware that Monaco's resize quirks haven't been completely ironed out. Still, it's a solid step forward!
For an illustrative example, check out 7d1ddec.

I hope this helps someone down the line!

heldersepu · 2024-04-10T11:14:45Z

@nojaf don't feel discouraged this PR is great, the syntax highlighter was in need of a major refactor...
The team must be busy with other priorities

heldersepu · 2024-04-10T11:15:58Z

When you can address the:
This branch has conflicts that must be resolved

char0n · 2024-04-10T12:54:31Z

Thanks for all the work on this, but we'll not be incorporating this major architectural change into core SwaggerUI. Due to #9783, this can now be approached using the standard SwaggerUI plugin system. More information in #3832 (comment)

I've also created a wiki page with your MonacoEditor plugin - https://github.com/swagger-api/swagger-ui/wiki/Plugins

nojaf · 2024-04-10T13:20:31Z

Hi @char0n, thanks for the update and for putting my experiment into the wiki!

char0n · 2024-04-10T13:29:42Z

@nojaf it's experiments like these that push the evolution of SwaggerUI forward. Your PR did directly inspire #9783

nojaf added 4 commits March 21, 2024 15:25

First attempt at replacing SyntaxHighlighter with Monaco Editor.

13ed033

Merge branch 'master' into large-bodies

9e8bbb9

Update unit test

c11b8f4

Merge remote-tracking branch 'origin/large-bodies' into large-bodies

662a9dc

nojaf mentioned this pull request Mar 21, 2024

Large response bodies cause hanging #3832

Open

heldersepu suggested changes Mar 21, 2024

View reviewed changes

src/core/syntax-highlighting.js Outdated Show resolved Hide resolved

test/unit/components/highlight-code.jsx Show resolved Hide resolved

nojaf added 2 commits March 25, 2024 09:06

Address code review feedback.

5c4d7f0

Merge master

6109100

nojaf marked this pull request as ready for review March 25, 2024 08:14

Merge branch 'master' into large-bodies

cb811f5

Build bundles and style

9c2d574

nojaf added 3 commits April 4, 2024 10:46

Merge branch 'master' into large-bodies

61447d3

Merge branch 'master' into large-bodies

7944131

Fix linting issues.

457e88a

nojaf added 2 commits April 4, 2024 17:48

Update theme setting to Monaco.

b4b797e

Build bundles

c689a4d

nojaf added 2 commits April 10, 2024 13:56

Merge branch 'master' into large-bodies

917089f

Replace ReactSyntaxHighlighter with Monaco

4263524

char0n self-assigned this Apr 10, 2024

char0n added Workaround available type: enhancement version: 5.x type: support labels Apr 10, 2024

char0n closed this Apr 10, 2024

heldersepu mentioned this pull request Jun 8, 2024

Multi-line selection/editing ether/etherpad-lite#6433

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace SyntaxHighlighter with Monaco Editor. #9728

Replace SyntaxHighlighter with Monaco Editor. #9728

nojaf commented Mar 21, 2024

heldersepu commented Mar 21, 2024 •

edited

Loading

heldersepu commented Mar 23, 2024 •

edited

Loading

heldersepu commented Mar 23, 2024 •

edited

Loading

nojaf commented Mar 25, 2024

heldersepu commented Mar 25, 2024

heldersepu commented Mar 25, 2024

nojaf commented Apr 3, 2024

heldersepu commented Apr 3, 2024

nojaf commented Apr 4, 2024

nojaf commented Apr 4, 2024

heldersepu commented Apr 5, 2024

nojaf commented Apr 10, 2024

heldersepu commented Apr 10, 2024

heldersepu commented Apr 10, 2024

char0n commented Apr 10, 2024

nojaf commented Apr 10, 2024

char0n commented Apr 10, 2024

Replace SyntaxHighlighter with Monaco Editor. #9728

Replace SyntaxHighlighter with Monaco Editor. #9728

Conversation

nojaf commented Mar 21, 2024

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Checklist

My PR contains...

My changes...

Documentation

Automated tests

heldersepu commented Mar 21, 2024 • edited Loading

heldersepu commented Mar 23, 2024 • edited Loading

heldersepu commented Mar 23, 2024 • edited Loading

nojaf commented Mar 25, 2024

heldersepu commented Mar 25, 2024

heldersepu commented Mar 25, 2024

nojaf commented Apr 3, 2024

heldersepu commented Apr 3, 2024

nojaf commented Apr 4, 2024

nojaf commented Apr 4, 2024

heldersepu commented Apr 5, 2024

nojaf commented Apr 10, 2024

heldersepu commented Apr 10, 2024

heldersepu commented Apr 10, 2024

char0n commented Apr 10, 2024

nojaf commented Apr 10, 2024

char0n commented Apr 10, 2024

heldersepu commented Mar 21, 2024 •

edited

Loading

heldersepu commented Mar 23, 2024 •

edited

Loading

heldersepu commented Mar 23, 2024 •

edited

Loading