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

103/vscodeplugin #460

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

Adwoa-Konadu-Appiah
Copy link
Contributor

Issue 103
An initial implementation to visualize CALM instantiation when CALM: Calm Visualize Current Instantiation File is run in the VS code command pallet.

@Adwoa-Konadu-Appiah Adwoa-Konadu-Appiah requested a review from a team as a code owner October 8, 2024 11:42
@rocketstack-matt
Copy link
Member

rocketstack-matt commented Oct 8, 2024

Thanks for the PR @Adwoa-Konadu-Appiah!

@aidanm3341 can we get your review on this PR please.

An initial thought from me, should we place this in a top level folder called plugins then vscode that way if we create extensions for IntelliJ or other tools they can be grouped together?

@Thels
Copy link
Member

Thels commented Oct 9, 2024

Thanks for the PR @Adwoa-Konadu-Appiah!

@aidanm3341 can we get your review on this PR please.

An initial thought from me, should we place this in a top level folder called plugins then vscode that way if we create extensions for IntelliJ or other tools they can be grouped together?

Completely agree - we should go with plugins -> vscode -> validate / visualize whatever

@Thels
Copy link
Member

Thels commented Oct 9, 2024

I think we should hold fire on merging this - it looks like the calmToDot was copied into this extension folder.

There are problems having the extension depend on the CLI module, but this is something I believe we really need to overcome prior to merging.

Copy link
Member

Choose a reason for hiding this comment

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

Ideally - this file, visualize.ts and calmToDot.ts aren't copy pasted into this directory for this to work.

I understand for a PoC this was done to produce an output, but we shouldn't merge in this state unless others disagree.

Copy link
Member

Choose a reason for hiding this comment

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

agreed. The approach we should take if possible is to pull in the CLI as a dependency in the package.json and pull in these files through that. As you mentioned in your other comment, there are some issues that come up regarding the mismatch between ESM and CJS, due to VSCode extensions only supporting CJS.

@Adwoa-Konadu-Appiah it would be good if you could also try doing it this way and see if you can solve the issues that come up

Copy link
Contributor Author

@Adwoa-Konadu-Appiah Adwoa-Konadu-Appiah Oct 10, 2024

Choose a reason for hiding this comment

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

I believe the optimal approach is to extract the logic that represents CALM as a graph into a separate npm package. This package can be maintained independently and utilized by the CLI, plugin, and any future requirements.
@Thels @rocketstack-matt

Copy link
Member

Choose a reason for hiding this comment

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

I agree, but I think that can wait until we have figured out the dependency issue.

It looks to be possible to have our VSCodeExtension package depend on the cli but it involves compiling the typescript within CLI into commonJS format which is icky, it's stop-gap until it handles importing ESM modules ( which does look to be on the horizon - microsoft/vscode#130367 )

Going to play around with this problem this weekend, as it's a major blocker to introducing the first IDE support for CALM which would be huge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Thels Has there been any update?

Copy link
Member

Choose a reason for hiding this comment

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

#476

I've raised another PR which covers quite a lot of changes but the two major ones in a nutshell are;

  • Introducing a shared module which both the plugin and cli depend on. ( you can see from the PR how much of a pain-point this was - a lot of the CLI code wasn't built to be used programmatically )
  • Plugin code which depends on this new shared module.

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.

4 participants