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

Extension rewrite #26

Merged
merged 8 commits into from
Mar 20, 2024
Merged

Extension rewrite #26

merged 8 commits into from
Mar 20, 2024

Conversation

JBBianchi
Copy link
Member

@JBBianchi JBBianchi commented Feb 26, 2024

Many thanks for submitting your Pull Request ❤️!

What this PR does / why we need it:
This is a complete rewrite of the extension. It simplifies & cleans the code and remove the dependency to an external server to render the graph.

Special notes for reviewers:
Fix #27

Additional information (if needed):

Signed-off-by: Jean-Baptiste Bianchi <[email protected]>
Signed-off-by: Jean-Baptiste Bianchi <[email protected]>
Copy link
Member

@ricardozanini ricardozanini left a comment

Choose a reason for hiding this comment

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

Looks neat! I'd like to see this in action now. Is there a way to install a snapshot?

* @tsurdilo @manuelstein @ricardozanini @JBBianchi
Copy link
Member

Choose a reason for hiding this comment

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

You can remove Manuel here and I'll give you write permissions here. :)

@JBBianchi
Copy link
Member Author

Looks neat! I'd like to see this in action now. Is there a way to install a snapshot?

I think you should be able to test it out using the Building from source instructions. The output file (vsix) will have a different name though, serverless-workflow-0.0.1.vsix if I'm not mistaken.

@ricardozanini
Copy link
Member

@JBBianchi I was able to export to SVG and preview, but for some reason, I couldn't for PNG. The preview pane just opens and shows the diagram when I pick this option. Is this expected?

@JBBianchi
Copy link
Member Author

@JBBianchi I was able to export to SVG and preview, but for some reason, I couldn't for PNG. The preview pane just opens and shows the diagram when I pick this option. Is this expected?

It's not, I'll investigate. Did the workflow you were trying to export exist on the disk or was it just a "virtual" file (tab) in VSCode but not yet saved on disk ?

@JBBianchi
Copy link
Member Author

I pushed a fix for "virtual files" but I think I faced a race condition on the first time I tried to debug but couldn't reproduce it.

@ricardozanini can you give me some feedbacks using the latest committed version ?

@ricardozanini
Copy link
Member

@JBBianchi I was able to export to SVG and preview, but for some reason, I couldn't for PNG. The preview pane just opens and shows the diagram when I pick this option. Is this expected?

It's not, I'll investigate. Did the workflow you were trying to export exist on the disk or was it just a "virtual" file (tab) in VSCode but not yet saved on disk ?

It exists! I'll try the new commit.

@ricardozanini
Copy link
Member

@JBBianchi same thing, the preview even flashes a little bit now, but won't generate the png. We can do a live session tomorrow, wdyt?

@JBBianchi
Copy link
Member Author

Sure, with pleasure.

@JBBianchi
Copy link
Member Author

I found a bug when generating the PNG. The process to generate the PNG is the following:

  • Generate the SVG using Mermaid
  • Grab the SVG, convert it to base64
  • Use the base64 as an img tag source
  • When the image has loaded, inject it in a canvas and export the canvas as PNG

There was a problem generating the base64 from the SVG in some cases (I guess bad character escaping). It should be fixed now. (If GitHub would be kind enough to sync the PR ...)

@ricardozanini
Copy link
Member

I'll retest again this afternoon! Thanks!

@ricardozanini
Copy link
Member

@JBBianchi still not working on my machine :(

@ricardozanini
Copy link
Member

Hey @JBBianchi I asked a friend (@handreyrc) to also test the extension and he used this file:

{
  "id": "statesExample",
  "version": "1.0",
  "specVersion": "0.8",
  "name": "States Example",
  "description": "States Example",
  "start": "Create Initial Data",
  "states": [
   {
    "name": "Create Initial Data",
    "type": "sleep",
    "end": false,
    "transition": "Inject data"
   },
   {
    "name": "Inject data",
    "type": "foreach",
    "end": false,
    "compensatedBy": "Compensate",
    "transition": "Wait for Events",
    "onErrors": [
     {
      "errorRef": "SomeError1",
      "end": false,
      "transition": "Handle Error1"
     },
     {
      "errorRef": "SomeError2",
      "end": false,
      "transition": "Handle Error2"
     }
    ]
   },
   {
    "name": "Handle Error1",
    "type": "inject",
    "end": true
   },
   {
    "name": "Handle Error2",
    "type": "inject",
    "end": true
   },
   {
    "name": "Wait for Events",
    "type": "event",
    "onEvents": [
     {
      "eventRefs": [
       "Event"
      ],
      "actions": [
       {
        "functionRef": "Function"
       }
      ]
     }
    ],
    "transition": "Run Operations",
    "end": false
   },
   {
    "name": "Run Operations",
    "type": "operation",
    "actionMode": "sequential",
    "actions": [
     {
      "name": "action1",
      "functionRef": "Task1"
     },
     {
      "name": "action2",
      "subFlowRef": "Task2"
     }
    ],
    "end": false,
    "transition": "Switch State"
   },
   {
    "name": "Switch State",
    "type": "switch",
    "onErrors": [
     {
      "errorRef": "SomeError3",
      "end": false,
      "transition": "Handle Error3"
     }
    ],
    "eventConditions": [
     {
      "eventRef": "event1",
      "transition": "Event1 State"
     },
     {
      "eventRef": "event2",
      "transition": "Event2 State"
     }
    ],
    "defaultCondition": {
     "transition": "Default State"
    }
   },
   {
    "name": "Handle Error3",
    "type": "callback",
    "end": true
   },
   {
    "name": "Event1 State",
    "type": "event",
    "end": true
   },
   {
    "name": "Event2 State",
    "type": "event",
    "end": true
   },
   {
    "name": "Default State",
    "type": "parallel",
    "end": true
   },
   {
    "name": "Compensate",
    "type": "callback",
    "end": true
   }
  ]
 }

Here's his output:

No the PNG export does not work, it opens a new preview tab for me and no png file is created.
I tested the SVG export as well, it seems it quickly creates a new tab and then closes it in the process. The SVG file was created but I cannot open it, it is broken.

@JBBianchi
Copy link
Member Author

Afaik, he is not able to preview it either because the TS SDK raises an error: Format not supported

@JBBianchi JBBianchi changed the title Extension rewrite [WIP] Extension rewrite Mar 7, 2024
@handreyrc
Copy link

handreyrc commented Mar 7, 2024

Afaik, he is not able to preview it either because the TS SDK raises an error: Format not supported

Hey @JBBianchi,

@ricardozanini posted a workflow not supported by this extension I mentioned in a talk with him. You are right this workflow won't work.
I apologize for the misunterstood!

In fact I tested this PR with many other worflows and it does not work.
Just giving you more background about the behavior above:
OS: Fedora Linux 38
VsCode version: 1.87.0

Here goes one of the workflows it fails:
greeting.sw.json

This is how it looks on my machine. I hope it helps you.
Extension_Test

@JBBianchi
Copy link
Member Author

JBBianchi commented Mar 8, 2024

Afaik, he is not able to preview it either because the TS SDK raises an error: Format not supported

Hey @JBBianchi,

@ricardozanini posted a workflow not supported by this extension I mentioned in a talk with him. You are right this workflow won't work. I apologize for the misunterstood!

In fact I tested this PR with many other worflows and it does not work. Just giving you more background about the behavior above: OS: Fedora Linux 38 VsCode version: 1.87.0

Here goes one of the workflows it fails: greeting.sw.json

This is how it looks on my machine. I hope it helps you. ...

Thanks for your report. I'll investigate.

@JBBianchi
Copy link
Member Author

JBBianchi commented Mar 8, 2024

@handreyrc I pushed an update that should provide more debug info. Could you please try again and tell me what error pops up ?

Two more little things that could bring some pieces of info:

  • switch to the explorer view
  • enable the Developer Tools console to see if some messages are displayed there (F1: Developer: Toggle Developer Tools)
    image

@JBBianchi
Copy link
Member Author

I fixed a possible race condition where the request to draw the graph would be sent before mermaid was important in the viewport (basically, the handler for the rendering request wouldn't be attached yet, therefore resulting in a blank page).

Besides that, which didn't seem to be the case in @handreyrc case as the graph was rendering, I didn't find anything else and it seems to be working on the latest Fedora:
fedora

@ricardozanini
Copy link
Member

@JBBianchi @handreyrc it's working now!!!

@JBBianchi JBBianchi changed the title [WIP] Extension rewrite Extension rewrite Mar 12, 2024
Signed-off-by: Jean-Baptiste Bianchi <[email protected]>
@ricardozanini
Copy link
Member

@cdavernas @antmendoza can you please review?

Copy link
Member

@cdavernas cdavernas left a comment

Choose a reason for hiding this comment

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

LGTM! Cheers ❤️

@ricardozanini
Copy link
Member

@antmendoza can you ptal?

@LuboTerifaj
Copy link

Hello @JBBianchi , @ricardozanini

is there any issue reported with the details what is its goal? I see only one bug-fix linked in the PR description. However, the PR's description looks like it has a wider scope than just to fix a bug.

I see there is no github action that would build the extension. Building it locally is the only option at the moment?

Thanks!

@ricardozanini
Copy link
Member

@LuboTerifaj there's none AFAIK. The extension won't work in the current context. @JBBianchi did the whole rewriting to make it work again. So what we are trying to do here is to test it locally and see if it's working. e.g. visualize the workflow and export to PNG/SVG.

@cdavernas Wanna take a look? After your approval, we can consider merging it. @JBBianchi can we follow up by fixing also the web editor? 🤔

@ricardozanini
Copy link
Member

@LuboTerifaj one more thing, we don't have a GHA for this extension. If you know about GHA, can you consider a PR? 🙏
thank you!

@cdavernas
Copy link
Member

@ricardozanini I have already approved it yesterday 😉

@JBBianchi
Copy link
Member Author

Hello @JBBianchi , @ricardozanini

is there any issue reported with the details what is its goal? I see only one bug-fix linked in the PR description. However, the PR's description looks like it has a wider scope than just to fix a bug.

I see there is no github action that would build the extension. Building it locally is the only option at the moment?

Thanks!

@LuboTerifaj > The first goal was to fix a bug reported on Slack. But when I tracked the bug I noticed 1st that the extension was relying on an external server to process the graph, which isn't optimal, and 2nd the code base was a bit scattered and not very ubiquitous. Therefore I decided to go for a full refactor, bringing the graph generation inside the extension.

At the moment, we didn't configure any pipeline to build the extension so there only solution to test it is to clone it and build it locally. But it's certainly possible to create a pipeline to do so. Please feel free to contribute one if you feel like it.

@LuboTerifaj there's none AFAIK. The extension won't work in the current context. @JBBianchi did the whole rewriting to make it work again. So what we are trying to do here is to test it locally and see if it's working. e.g. visualize the workflow and export to PNG/SVG.

@cdavernas Wanna take a look? After your approval, we can consider merging it. @JBBianchi can we follow up by fixing also the web editor? 🤔

What fix for the web editor are you talking about @ricardozanini ? (Besides updating the TS SDK. It's not related to this VSCode extension.)

Other general note, there is no tests for the extension. I didn't dig a lot but at first glance I couldn't find a proper e2e testing example for VSCode.

@ricardozanini
Copy link
Member

@ricardozanini I have already approved it yesterday 😉

LOL

@ricardozanini
Copy link
Member

@JBBianchi I thought the web editor in the website would share the same libs/framework as the extension. My bad then! Can we merge this?

@JBBianchi
Copy link
Member Author

@JBBianchi I thought the web editor in the website would share the same libs/framework as the extension. My bad then! Can we merge this?

They do, the SDK and Mermaid :) We can merge.

@ricardozanini ricardozanini merged commit 5769100 into serverlessworkflow:main Mar 20, 2024
1 check passed
@LuboTerifaj
Copy link

Just fyi, the user experience is not very good when you try to open more workflows in a short time. It is very time consuming to open the diagram using Palette again and again. I have reported the issue here: #29

@ricardozanini
Copy link
Member

Thank you @LuboTerifaj!

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.

An error is raised when generating a png
5 participants