WIP: Add GLSP DiagramEditor extension #111

max-elia · 2021-06-07T08:13:12Z

Implement generator for glsp theia extension based on https://github.com/eclipse-glsp/glsp-examples
Give the possibility to add additional custom nodes with the shape of Rectangle, Circle or Diamond
Closes Support generator for glsp extension #110

Signed-off-by: Max Elia Schweigkofler max_elia@hotmail.de

JonasHelming

There are lots of unrelated formatting changes, e.g. semicolon and changes of ' to "
Could you remove them from the PR please?

JonasHelming · 2021-06-07T08:16:12Z

Could you also rebase and resolve the conflicts, please?

JonasHelming · 2021-06-07T08:17:14Z

Also I guess your state of master is old, so the automatic build is not yet in...

max-elia · 2021-06-07T09:01:35Z

There are lots of unrelated formatting changes, e.g. semicolon and changes of ' to "
Could you remove them from the PR please?

Removed formatting changes, except made all strings consistent with single quote ' and \' if inside string.

Could you also rebase and resolve the conflicts, please?

Done.

vince-fugnitto

I'm not really sure about the example, I'll have to think more about it:

it relies heavily on an external repo and project, it might be better not to include it as we now have a dependency on a downstream project which will be harder to maintain.
the project structure is not really consistent with other theia applications.
it is a very heavy example that needs to install java and maven.

It might be better to have an eclipse-glsp generator rather than trying to fit it into the theia generator.

cc @marcdumais-work

vince-fugnitto · 2021-06-07T16:47:20Z

README.md

@@ -51,6 +51,7 @@ yo theia-extension --help
 | `tree-editor` | Creates a tree editor extension | [readme](https://github.com/eclipse-theia/generator-theia-extension/blob/master/templates/tree-editor/README.md) |
 | `empty` | Creates a simple, minimal extension | [readme](https://github.com/eclipse-theia/generator-theia-extension/blob/master/templates/empty/README.md) |
 | `backend` | Creates a backend communication extension | [readme](https://github.com/eclipse-theia/generator-theia-extension/blob/master/templates/backend/README.md) |
+| `diagram-editor` | Creates a diagram editor extension | [readme](https://github.com/eclipse-glsp/glsp-examples/blob/master/README.md) |


We should probably not reference links to other repos, especially those not in the same organization.
We can probably reference a readme as part of the repo (maintained by the repo).

vince-fugnitto · 2021-06-07T16:47:56Z

package.json

@@ -16,6 +16,9 @@
  "author": "TypeFox",
  "license": "EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0",
  "dependencies": {
+    "download": "^8.0.0",
+    "fs-extra": "^10.0.0",
+    "github-download": "^0.5.0",


Is the dependency being used?

@max-elia please check

Removed it.

vince-fugnitto · 2021-06-07T16:48:59Z

src/app/index.ts

+        /** DiagramEditor */
+        if (this.params.extensionType === ExtensionType.DiagramEditor) {
+            var done = this.async();
+            download('https://github.com/eclipse-glsp/glsp-examples/archive/master.tar.gz', 'tmp', {


I'm not really fond of depending on external repos this way. I'm not sure this is appropriate for a theia extension generator.

I agree this is not ideal. For example we have the vscode-ripgrep dependency in the main repo that does this, to fetch a platform-appropriate ripgrep binary from GH downloads, and is a rather consistent source of misc problems, like getting the download to work behind some corporate firewalls.

Do we know if the download dependency will respect proxy environment settings?

Exchanged the download dependency and replaced it with the more common request and tar packages.

JonasHelming · 2021-06-08T10:00:54Z

I'm not really sure about the example, I'll have to think more about it:

it relies heavily on an external repo and project, it might be better not to include it as we now have a dependency on a downstream project which will be harder to maintain.

the project structure is not really consistent with other theia applications.

it is a very heavy example that needs to install java and maven.

It might be better to have an eclipse-glsp generator rather than trying to fit it into the theia generator.

cc @marcdumais-work

@vince-fugnitto : Thank you for your comment. We actually discussed the exact same concerns before we decided to contribute this here. The example is essentially showing how GLSP diagrams can be embedded into Theia. So you can definitily argue it should be located here or in a GLSP specific generator. Our main drivers to contribute here were:

It is visible to Theia beginners, which i believe is positive for Theia and GLSP. It shows what you can do with Theia and that there are other technologies integrating with it.
If you get started with Theia and a diagram, you likely also want a command, a tree or others things in this generator, so for the user it feels more consistent than using to (or maybe more) generators.
Strictly speaking, other generators have technology dependencies, too, e.g. the widget depends on React.
We wanted to avoid maintaining two generators. We actually took over more of the maintainance work of the Theia generator and intend to continue to do so. Therefore, we can put 100% of our maintainance resource on one generator, rather than splitting it. This would not be a good argument on its own, but combined with the others, I believe it is valid.
Finally, and most important, we obeserve that especially new adopters get confused about the project borders. We frenquently get questions and comments that the available technologies are not easy to get and the relations between them are hard to find. I believe having a place with examples that integrate the different projects into Theia can be a part of a solution to this issue.

For the maintenance effort, I can commit to heavily contribute to this.

At the end, both options have their pros and cons, I hope the reasons above make some sense to you.

vince-fugnitto · 2021-06-08T12:13:59Z

@JonasHelming I don't mean to hold back the pull-request, just wanted to share my thoughts on it candidly (as is I have issues with the extension directly pulling files from another repository). I'll let @marcdumais-work take a look as well, initially he shared the same sentiments I had.

It is visible to Theia beginners, which i believe is positive for Theia and GLSP. It shows what you can do with Theia and that there are other technologies integrating with it.

While true, I don't think we should aim to include all possible downstream examples as part of the generic generator.

If you get started with Theia and a diagram, you likely also want a command, a tree or others things in this generator, so for the user it feels more consistent than using to (or maybe more) generators.

I believe the generated templates should be simple, as you mentioned it is for beginners (some have issues with just the hello-world example). More complex templates should likely not be part of the repo, and the glsp + theia example can simply be its own repo which has an example on how to do the integration. But this is my opinion.

Strictly speaking, other generators have technology dependencies, too, e.g. the widget depends on React.

I believe the two are not the same, it is different to have a versioned dependency which is heavily tested and maintained like React and instead fetch files from another repository. In addition, the template always pulls master which is not ideal. I would not say this point is valid as is.

We wanted to avoid maintaining two generators. We actually took over more of the maintainance work of the Theia generator and intend to continue to do so. Therefore, we can put 100% of our maintainance resource on one generator, rather than splitting it. This would not be a good argument on its own, but combined with the others, I believe it is valid.

I believe point 1 applies here.

Finally, and most important, we obeserve that especially new adopters get confused about the project borders. We frenquently get questions and comments that the available technologies are not easy to get and the relations between them are hard to find. I believe having a place with examples that integrate the different projects into Theia can be a part of a solution to this issue.

The use of examples also succeed in informing users, I don't necessarily believe it has to be part of a generator.

If we are to move forward with this generator, we should fix the following:

not fetch files directly from the other repository.
not fetch master (as we have no guarantee of what is added to master and it is the opposite of versioning).

In essence, having such a tight coupling with the other repository is not ideal.

JonasHelming · 2021-06-08T12:29:53Z

thank you again for sharing your opinion!

Quick note: I definitely share the concern with fetching master, it should be a tag which is actively maintained for this purpose

"While true, I don't think we should aim to include all possible downstream examples as part of the generic generator."
=> I think we are far away from this state :-)

marcdumais-work · 2021-06-17T15:49:29Z

I'm not really sure about the example, I'll have to think more about it:

FWIW, I think this is a rather niche example at the moment, that most Theia extension developers will not need. However, the few that do will benefit from it being available. So, as long as this new example does not hinder too much, I think it's worthwhile, and could kick-start a nice eco-system of Theia-based graphical/diagram apps.

marcdumais-work · 2021-06-17T16:13:38Z

In essence, having such a tight coupling with the other repository is not ideal.

However, if I understand correctly, the dependency seems well-insulated, in the sense that it will be fetched lazily, when the user chooses to create that type of extension. IoW if it breaks, the rest (other examples) should still work? That, and the commitment by Jonas to maintain it, appeases my mild apprehensions about this PR.

In addition, it will be a nice example to have, no? Very different from what we have so far in this repo?

max-elia · 2021-07-26T12:31:06Z

The diagram-editor example now fetches from a specific tag.

cc @JonasHelming

JonasHelming · 2021-09-12T11:36:32Z

@vince-fugnitto : I would like to merge this, any objections?

vince-fugnitto · 2021-09-13T12:23:08Z

@vince-fugnitto : I would like to merge this, any objections?

@JonasHelming I don't have strong objections but the glsp extension should likely be updated upstream, there are a few issues such as:

Pinning all of theia to 1.14.0
Because of the pin, the extension still targets es5 and not es2017
The idea of client and server (top level directories) does not make sense to me, can you clarify why it's done this way? The "client" component (theia) is made up of client + server already so it may be confusing to adopters.
Given that the extension is one of the more complex templates we have as part of the repository, we might want to document the code more, the example is likely not documented well enough for adopters to use as is.

JonasHelming · 2021-09-14T09:03:30Z

Thank you very much for your feedback, I created upstream issues!

- Implement generator for glsp theia extension based on https://github.com/eclipse-glsp/glsp-examples - Give the possibility to add additional custom nodes with the shape of Rectangle, Circle or Diamond Closes #110 Signed-off-by: Max Elia Schweigkofler <max_elia@hotmail.de>

planger · 2022-08-09T08:00:32Z

We've re-worked this generator and opened a new PR: #153
I think this one can now be closed.

JonasHelming requested changes Jun 7, 2021

View reviewed changes

vince-fugnitto reviewed Jun 7, 2021

View reviewed changes

JonasHelming self-requested a review September 12, 2021 11:35

JonasHelming approved these changes Sep 12, 2021

View reviewed changes

This was referenced Sep 14, 2021

[Minimal example] Pinned to version 1.14.0 eclipse-glsp/glsp#387

Closed

[Minimal example] Does the structure /client /sever make sense? eclipse-glsp/glsp#388

Closed

[Minimal example] Add more documentation eclipse-glsp/glsp#389

Closed

planger mentioned this pull request Oct 7, 2021

Add yeo generator for minimal example eclipse-glsp/glsp#113

Closed

max-elia changed the title ~~Add GLSP DiagramEditor extension~~ WIP: Add GLSP DiagramEditor extension Mar 30, 2022

JonasHelming closed this Aug 9, 2022

JonasHelming mentioned this pull request Aug 10, 2022

Add GLSP diagram editor extension #153

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Add GLSP DiagramEditor extension #111

WIP: Add GLSP DiagramEditor extension #111

max-elia commented Jun 7, 2021

JonasHelming left a comment

JonasHelming commented Jun 7, 2021

JonasHelming commented Jun 7, 2021

max-elia commented Jun 7, 2021 •

edited

Loading

vince-fugnitto left a comment

vince-fugnitto Jun 7, 2021

vince-fugnitto Jun 7, 2021

JonasHelming Jun 18, 2021

max-elia Jul 26, 2021

vince-fugnitto Jun 7, 2021

marcdumais-work Jun 17, 2021

max-elia Jul 26, 2021

JonasHelming commented Jun 8, 2021

vince-fugnitto commented Jun 8, 2021

JonasHelming commented Jun 8, 2021

marcdumais-work commented Jun 17, 2021

marcdumais-work commented Jun 17, 2021 •

edited

Loading

max-elia commented Jul 26, 2021

JonasHelming commented Sep 12, 2021

vince-fugnitto commented Sep 13, 2021

JonasHelming commented Sep 14, 2021

planger commented Aug 9, 2022

WIP: Add GLSP DiagramEditor extension #111

WIP: Add GLSP DiagramEditor extension #111

Conversation

max-elia commented Jun 7, 2021

JonasHelming left a comment

Choose a reason for hiding this comment

JonasHelming commented Jun 7, 2021

JonasHelming commented Jun 7, 2021

max-elia commented Jun 7, 2021 • edited Loading

vince-fugnitto left a comment

Choose a reason for hiding this comment

vince-fugnitto Jun 7, 2021

Choose a reason for hiding this comment

vince-fugnitto Jun 7, 2021

Choose a reason for hiding this comment

JonasHelming Jun 18, 2021

Choose a reason for hiding this comment

max-elia Jul 26, 2021

Choose a reason for hiding this comment

vince-fugnitto Jun 7, 2021

Choose a reason for hiding this comment

marcdumais-work Jun 17, 2021

Choose a reason for hiding this comment

max-elia Jul 26, 2021

Choose a reason for hiding this comment

JonasHelming commented Jun 8, 2021

vince-fugnitto commented Jun 8, 2021

JonasHelming commented Jun 8, 2021

marcdumais-work commented Jun 17, 2021

marcdumais-work commented Jun 17, 2021 • edited Loading

max-elia commented Jul 26, 2021

JonasHelming commented Sep 12, 2021

vince-fugnitto commented Sep 13, 2021

JonasHelming commented Sep 14, 2021

planger commented Aug 9, 2022

max-elia commented Jun 7, 2021 •

edited

Loading

marcdumais-work commented Jun 17, 2021 •

edited

Loading