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

fix: add the suggested incorrect spec preview changes #230

Open
wants to merge 20 commits into
base: feature/mermaid-markdown
Choose a base branch
from

Conversation

nikhilkalburgi
Copy link
Contributor

Description

  • I have improved the preview by adding the feature to render incorrect specs. It will reneder the correct part of the spec and on top will also show the incorrect part as well.

Check this demo:

20.05.2024_21.46.13_REC-ezgif.com-video-cutter.mp4
  • In next iteration, i will start to build the preview support for avro and then also the flowchart.
  • Let me know your feedback

@ivangsa
Copy link
Collaborator

ivangsa commented May 21, 2024

How do you open this preview, I can only get this view (from your branch)
image

@nikhilkalburgi
Copy link
Contributor Author

For the merged Pull Request,
There is one issue in package.json which I have sorted in this PR.
image

I did the cloning of feature/mermaid-markdown branch, then did npm install and finally executed it with F5 or Run button.

It works for me.

image

In this PR, I have made far more changes like separating the CSS to a new file called globals.css. Removed the following 2 files from ./components folder.
image

Note: the globals.css do not update live and needs to be deleted from ./dist after any change in it.

@ivangsa
Copy link
Collaborator

ivangsa commented May 21, 2024

what is the local branch in your repo? I'm using this one, and still don't get the preview
image

@nikhilkalburgi
Copy link
Contributor Author

Sorry! A small code went missing in the webpack config file and was causing this issue. I have committed the latest change in the same mermaid-markdown branch. Can you please check again?

@ivangsa
Copy link
Collaborator

ivangsa commented May 30, 2024

Sorry for the delay answering, I can't still see this styles working..

I've tried packaging the extension and intalling it locally and still doesn't work for me..

image

They look great in your screenshot.. Keep up the good work!

@ivangsa
Copy link
Collaborator

ivangsa commented May 30, 2024

check this out:
image

@nikhilkalburgi
Copy link
Contributor Author

image
image

  1. I am changing the directory, installing the dependencies and opening in VSCode
    image

  2. In VSCode, I run the project using the run button
    image

Project Structure Before:
image

After the Build the new ./dist folder will be:
image

In the Ext tab:
image

@nikhilkalburgi
Copy link
Contributor Author

In my build process there are 12 modules being installed

image

@ivangsa
Copy link
Collaborator

ivangsa commented May 30, 2024

I just reproduced your same steps... and I'm still getting this error :-(

image

image

@nikhilkalburgi
Copy link
Contributor Author

I noticed that in your case the extension in not able to access two files: globals.css and mermaid.min.js.

In PreviewMarkdown.ts, I have added the specific files to the localResources like:
image

But, I think we need to include the resource directory of the these 2 file instead.
image

This is how it is also done in PreviewWebPanel.ts:
image
standalone and styles are the directory from which the below 2 files are extracted and used in the HTML preview:
image

Can you try doing this?

We need to authorize these two files somehow

@nikhilkalburgi
Copy link
Contributor Author

Hi Ivan,

I have updated this PR also to include support for the AVRO syntax preview. Are you still not able to access those two files?

@ivangsa
Copy link
Collaborator

ivangsa commented Jun 5, 2024

beautiful!! keep on working this way!
image

  • If you create a first version for the flowchart and class diagram tabs we can think about merging this into main...

  • Also we would need to think about how to address the two "open preview" buttons. Do you have anything in mind for this?

If you need any help or guidance just let me know...

@nikhilkalburgi
Copy link
Contributor Author

I have already added both preview-type commands and title bar icons. I will change the icon for the markdown. It would look something like the following:

image

What do you think?

@nikhilkalburgi
Copy link
Contributor Author

Hi @ivangsa ,

I have finally implemented the Flowchart and ClassDiagram with this PR. With this:

  1. Added the mermaid.js support to render the diagram
  2. Using panzoom.js to create pan and zoom effects on the diagrams.
  3. Improved the rendering performance on markdown and diagrams.

Have a look:

11.06.2024_13.08.07_REC.mp4

Nextly, I will:

  1. Work on Markdown Export.
  2. Debug with multiple test cases.

@nikhilkalburgi
Copy link
Contributor Author

Hi @ivangsa,

How are you doing?

Updating you after a long time. I have improved the rendering of incorrect spec to a better extent and also integrated the export to markdown button to top-right corner of the webview. This new button will copy the MD version of preview to clipboard.

The preview supports both the V3 & V2 version for Incorrect Spec.

@ivangsa
Copy link
Collaborator

ivangsa commented Jul 9, 2024

@nikhilkalburgi great!!

I hope to have some time this week to review your work..

cheers!

@ivangsa
Copy link
Collaborator

ivangsa commented Jul 11, 2024

hi @nikhilkalburgi

I run it on me computer and it looks very promising, I have a few suggestions:

  • Markdown: Looks great but I think it need a clickable index on top so we can navigate through the document: try different approaches: a) operations -> channels -> messages -> schemas b) channels -> messages -> operations...
  • Flowchart: I'm not sure how much control we have about the layout but a couple of suggestions:
    • make messages something like a nested object inside channels
    • add PUB/SUB send/receive indicators to operations
  • Class Diagram: does not work for me, it show a diagram that does not represent my model at all

image

I would expect something more like
image

I understand that bootstraping the code to this point it was a hard work on your side, now with a bit of polishing we can make this to production...

Keep on updating because we are almost there, and thanks for all your hard work!

@nikhilkalburgi
Copy link
Contributor Author

Hi @ivangsa,

Sorry for the late response! I have some doubts and info on your suggestions:

1. Markdown:

Can you share with me a mockup UI design on a real sample spec for a better understanding of indexing?

2. Flowchart:

  • I tried using different options in Mermaid but there is no nested object feature in Mermaid.js. I also tried converting the channel into a subgraph but could not create a relationship between a subgraph and the operation. The long channel name does not even fit in the node as it only spans for the message nested in it:

image

  • I will add the SUB/PUB | send/receive sign on every operation ✅

3. Class Diagram:

Currently, I am showing the hierarchy of message->payload->properties->propOne.
Example:

Channel:

'smartylighting/streetlights/1/0/event/{streetlightId}/lighting/measured':
    address: 'smartylighting/streetlights/1/0/event/{streetlightId}/lighting/measured'
    messages:
      receiveLightMeasurement.message:
        $ref: '#/components/messages/lightMeasured'
    description: The topic on which measured values may be produced and consumed.
    parameters:
      streetlightId:
        $ref: '#/components/parameters/streetlightId'

Operation:

receiveLightMeasurement:
    action: receive
    channel:
      $ref: >-
        #/channels/smartylighting~1streetlights~11~10~1event~1{streetlightId}~1lighting~1measured
    summary: >-
      Inform about environmental lighting conditions of a particular
      streetlight.
      . . . 

Message:

 lightMeasured:
      name: lightMeasured
      title: Light measured
      summary: >-
        Inform about environmental lighting conditions of a particular
        streetlight.
      contentType: application/json
      tags:
        . . .
      traits:
        - $ref: '#/components/messageTraits/commonHeaders'
      payload:
         $ref: '#/components/schemas/lightMeasuredPayload'

lightMeasuredPayload (Schema)

  type: object
    properties:
      lumens:
        type: integer
        minimum: 0
        description: Light intensity measured in lumens.
        x-pi: false
      sentAt:
        $ref: '#/components/schemas/sentAt'

Then, the same spec is represented as:

image

So, the period ('.') separated string represents the hierarchy from the top to the bottom. For example: lightMeasured.payload.lightMeasuredPayload.properties. where lightMeasured=message, payload= property in message, lightMeasuredPayload= schema used by payload, properties= property in schema.

If you are expecting something else then can you also use the same example and create a Mockup classDiagram on it?

@ivangsa
Copy link
Collaborator

ivangsa commented Aug 16, 2024

Hi @nikhilkalburgi , sorry for the late response

  1. By index, I mean a summary with links to the different sections: channels, messages, operations..

  2. I couldn't figure out how the hiearchy you are showing is working..
    Have a look at this openapi (it's only about the schemas which is the same as in asyncapi)

https://github.com/EDALearn/EDA-Playground-Online-Food-Delivery/blob/main/modules/orders/src/main/resources/apis/openapi.yml

image

This is what I would try to generate with mermaid for that schema:

image

(it may not be exact, because of the api version screenshot, but I think it's easy to get the idea)

I'll be mostly of line next days, but let me know if you need any help

@nikhilkalburgi
Copy link
Contributor Author

Hi @ivangsa,

I have made changes to class diagram and this is how it looks:
image

For the following objects:

CHANNEL:

'smartylighting/streetlights/1/0/event/{streetlightId}/lighting/measured':
    address: 'smartylighting/streetlights/1/0/event/{streetlightId}/lighting/measured'
    messages:
      receiveLightMeasurement.message:
        $ref: '#/components/messages/OrderEventMessage'
    description: The topic on which measured values may be produced and consumed.
    parameters:
      streetlightId:
        $ref: '#/components/parameters/streetlightId'

Operation:

 receiveLightMeasurement:
    action: receive
    channel:
      $ref: >-
        #/channels/smartylighting~1streetlights~11~10~1event~1{streetlightId}~1lighting~1measured
    summary: >-
      Inform about environmental lighting conditions of a particular
      streetlight.
    description: |
      This is the description with **bold** text.

      On multiple lines.
    tags:
      - name: oparation-tag1
        externalDocs:
          description: External docs description 1
          url: 'https://www.asyncapi.com/'
      - name: oparation-tag2
        description: Description 2
        externalDocs:
          url: 'https://www.asyncapi.com/'
      - name: oparation-tag3
      - name: oparation-tag4
        description: Description 4
      - name: message-tag5
        externalDocs:
          url: 'https://www.asyncapi.com/'
    traits:
      - $ref: '#/components/operationTraits/kafka'
    messages:
      - $ref: '#/channels/smartylighting~1streetlights~11~10~1event~1{streetlightId}~1lighting~1measured/messages/receiveLightMeasurement.message'

Message:

 OrderEventMessage:
      name: lightMeasured
      title: Light measured
      summary: >-
        Inform about environmental lighting conditions of a particular
        streetlight.
      contentType: application/json
      tags:
        - name: message-tag1
          externalDocs:
            description: External docs description 1
            url: 'https://www.asyncapi.com/'
        - name: message-tag2
          description: Description 2
          externalDocs:
            url: 'https://www.asyncapi.com/'
        - name: message-tag3
        - name: message-tag4
          description: Description 4
        - name: message-tag5
          externalDocs:
            url: 'https://www.asyncapi.com/'
      traits:
        - $ref: '#/components/messageTraits/commonHeaders'
      payload:
        $ref: '#/components/schemas/orderEvent'

Payload Schema:

orderEvent:
      properties:
        custerDetails_id:
            properties:
              customerId:
                type: "string"
              firstName:
                type: "string"  

Can you check it from your side and let me know if it works?
In parallel, I will start working on indexing.

@nikhilkalburgi
Copy link
Contributor Author

Hi @ivangsa ,

I was planning to build the index like the below image:
image

What do you think about this?

@ivangsa
Copy link
Collaborator

ivangsa commented Aug 31, 2024

Nice, dont forget to add channels and messages to index as well

Copy link

sonarqubecloud bot commented Sep 3, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
8.9% Duplication on New Code (required ≤ 3%)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

@nikhilkalburgi
Copy link
Contributor Author

Hi @ivangsa, I have also implemented the indexing in my latest commit.

@nikhilkalburgi
Copy link
Contributor Author

nikhilkalburgi commented Oct 7, 2024

Hi @ivangsa

I sincerely apologize. I'm always eager to contribute to AsyncAPI, and I deeply appreciate the support you've shown me, which I will value for the rest of my life. I wanted to revisit the GitHub thread and discuss why it's not quite ready for merging.

I have dedicated multiple hours to finishing the task that was not even part of the proposal. Just to give the best I can

We haven't had the chance to connect over a video call, and this may have led to some misunderstandings. Could we connect sometime? I’d love to hear what your expectations were so I can better understand and align with them.

Looking forward to your thoughts.

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

Successfully merging this pull request may close these issues.

3 participants