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

Bicep visualizer #2357

Merged
merged 21 commits into from
May 7, 2021
Merged

Bicep visualizer #2357

merged 21 commits into from
May 7, 2021

Conversation

shenglol
Copy link
Contributor

@shenglol shenglol commented Apr 22, 2021

Not something we planned, but I've been working on implementing this for a while as a side project during my spare time, and it finally gets to a point where I can submit a PR.

Main features:

  • Fault-tolerant resource dependency graph rendering
  • Beautiful layout
  • Error highlighting
  • Live update

To try it out:

  • Check out this branch
  • Under root, run dotnet build
  • cd ./src/vscode-bicep
  • npm i
  • code .
  • Press CTRL + F5 to launch a new window of VSCode
  • Open a Bicep file
  • Click the icon shown below at the top-right corner
    image

Demos:
bicep-viz-layout

bicep-viz-live-update

Closes #2358.
Closes #2290

rules: [
{
test: /\.ts$/,
loader: "esbuild-loader",
Copy link
Contributor Author

@shenglol shenglol Apr 22, 2021

Choose a reason for hiding this comment

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

Replacing ts-loader with esbuild-loader reduces by half the bundling time. For now we want to keep webpack because we need the plugins, but I'll investigate replacing it completely with esbuild (which is about 100x faster).

<polygon fill="#FFD289" points="64.11,71.99 64.04,98.07 41.33,84.97 41.41,58.88"/>
<polygon fill="#FFF1DC" points="86.67,58.88 64.11,71.99 41.41,58.88 63.96,45.77"/>
</g>
</svg>
Copy link
Contributor Author

@shenglol shenglol Apr 22, 2021

Choose a reason for hiding this comment

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

The two icons are created by me to avoid license issues.

I was thinking about using the official Azure architecture icons, but there is Terms of Use for those icons, which might affect our license. We'll need to figure it out later.

Copy link
Member

Choose a reason for hiding this comment

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

Can we access the VS code icons somehow? Could be an alternative?

We could also get new icons designed specifically for the Bicep visualizer.

Copy link
Contributor Author

@shenglol shenglol Apr 26, 2021

Choose a reason for hiding this comment

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

No, we cannot access VS code icons inside a webview, plus VS code doesn't provide icons for ARM providers and resources. AFAIK there's no good alternative for Azure icons, besides, I think we should probably not use something not official to represent MS products. As for designing our own icons, we can definitely do that for general UI icons, but it might not be practical to do it for so many RPs and resources.

I've attached the the terms of use of the Azure architecture icons, which I think won't block us from using it in a resource dependency graph (still need to confirm with their team).

However, my real concern is that no matter what SVG files we check in, they will become open source under MIT license. The office team seems to have the same issue with their fluentui repo, and their solution is to add a supplementary assets license agreement to the README license section, so maybe we can do the same thing. Thoughts?

image

Copy link
Member

Choose a reason for hiding this comment

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

Architecture icons seem like a reasonable option. My suggestion for designing new ones was only about replacements for the ones you did yourself. I definitely wouldn't want us to try to create 100s of RP icons.

@MarcusFelling
Copy link
Collaborator

👏

[Method("textDocument/deploymentGraph", Direction.ClientToServer)]
public record BicepDeploymentGraphParams(TextDocumentIdentifier TextDocument) : ITextDocumentIdentifierParams, IRequest<BicepDeploymentGraph?>;

public record BicepDeploymentGraph(IEnumerable<BicepDeploymentGraphNode> Nodes, IEnumerable<BicepDeploymentGraphEdge> Edges, int ErrorCount);
Copy link
Member

Choose a reason for hiding this comment

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

record

Ooh records!

type = type.split("/").pop() ?? type;
type += isCollection ? "[]" : "";

const backgroundSvg = `<?xml version="1.0" encoding="UTF-8"?><!DOCTYPE svg>
Copy link
Member

Choose a reason for hiding this comment

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

Why not just use tsx here? Wouldn't that remove the need for manual xml escaping?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we are using a vanilla JS library (cytoscape.js) to render the graph and this is the way to set custom SVG background for nodes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PS: we are using cytoscape because it is capable of rendering compound/nested graph. Eventually I would like to drop the dependency on cytoscape and implement our own graph renderer as a React component, but that's something we can do in the future as it requires quite a lot of work (or, we use an existing React library like react-flow, once they support rendering compound/nested graph).

@anthony-c-martin
Copy link
Member

You're definitely going to need to add a number of end-to-end tests for this functionality - including the client-side stuff like opening/closing panes etc.

Comment on lines 7 to 11
const backgroundColor = "#333333";
const backgroundColorSecondary = "#3f3f3f";
const foregroundColor = "#ffffff";
const foregroundColorSecondary = "#9c9c9c";
const errorColor = "red";
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to inherit colors from the user theme rather than hard-coding them? Do we need to think about accessibility here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question - technically we can read the colors from the user theme and send them to the webview, but there are concerns though:

  1. A color theme usually contains numerous background and foreground color definitions for different UI areas. It will be hard to choose what colors to use.
  2. We don't know what theme a user will use and if the graph will look good in that theme.

I'm leaning towards defining our own themes for the graph. This PR adds a dark theme, but later we can add a light theme and a high contrast theme (which addresses the accessibility issue).

Copy link
Member

Choose a reason for hiding this comment

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

Is there no standardization for how the colors are used? My assumption is that colors used for background vs foreground would have enough contrast to let people tell them apart.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes good call. There is Web Content Accessibility Guidelines (WCAG). Basically I think we'll want to meet the level AA success contrast criteria for the dark/light themes, and level AAA for the high contrast theme. There's an online tool to test that: https://webaim.org/resources/contrastchecker/.
I realized the foregroundColorSecondary is a bit too dark which does not pass the level AA test. I'll make some adjustment.

Copy link
Member

Choose a reason for hiding this comment

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

But even beyond that. VS code's own controls must be mapping these colors to themes somehow. Or rather each color "identifier" in VS code should have an intended usage (background, or various flavors of foreground colors). If we reuse those colors and match the intent of the particular color, wouldn't we get something that is theme-agnostic?

For example this tree view will always render ok no matter which theme you have (unless it's a really bad one):
image

I could be missing something here, though.

Copy link
Contributor Author

@shenglol shenglol May 6, 2021

Choose a reason for hiding this comment

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

To create a VSCode theme you need to define colors for each UI controls individually, so it's basically a 1:1 mapping. I think the challenge is, because the colors are not designed for rendering a graph, it's pretty hard to map them to our graph components. For example, what color we should use as the canvas background? What color should we use as the graph node background? sideBar.background or panel.background? In some themes, those background colors are different (such as the built-in dark theme), while in other themes they can be the same (like the one I use):

image

I'm not against this approach as it avoids designing our own color scheme, and it provides a more consistent UX, but I doubt we can find a theme-agnostic mapping between theme colors and the graph components.


const GlobalStyle = createGlobalStyle`
body {
font-family: Segoe UI, Helvetica Neue, Helvetica, sans-serif;
Copy link
Member

Choose a reason for hiding this comment

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

Are there any guidelines from VSCode about fonts? Any way to match this up with the theme?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIK there's no guidelines for fonts to use in webviews. We can read --vscode-editor-font-family in a webview but that's the monospaced font for code. That being said, I found the following comment in VSCode's source code, and I think we can use the same fonts in our webview:

/**
 * The best font-family to be used in CSS based on the platform:
 * - Windows: Segoe preferred, fallback to sans-serif
 * - macOS: standard system font, fallback to sans-serif
 * - Linux: standard system font preferred, fallback to Ubuntu fonts
 *
 * Note: this currently does not adjust for different locales.
 */
export const DEFAULT_FONT_FAMILY = isWindows ? '"Segoe WPC", "Segoe UI", sans-serif' : isMacintosh ? '-apple-system, BlinkMacSystemFont, sans-serif' : 'system-ui, "Ubuntu", "Droid Sans", sans-serif';

Copy link
Member

Choose a reason for hiding this comment

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

If people ask, we can always expose as a setting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah good pt.

<head>
<meta charset="UTF-8">
<!--
Use a content security policy to only allow loading images from https or from our extension directory,
Copy link
Member

Choose a reason for hiding this comment

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

Are you loading any external assets? If so, does this mean the functionality won't work without an internet connection?

Copy link
Contributor Author

@shenglol shenglol Apr 27, 2021

Choose a reason for hiding this comment

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

No we are not loading any external assets. Adding a CSP will only block resources not comply with the policy, which should be orthogonal to internet connections.

Comment on lines 202 to 210
private static getNonce() {
let text = "";
const possible =
"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789";
for (let i = 0; i < 32; i++) {
text += possible.charAt(Math.floor(Math.random() * possible.length));
}
return text;
}
Copy link
Member

Choose a reason for hiding this comment

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

Unless there's some tangible security benefit from doing this (I'm assuming not since you're not using a crypto algorithm to generate the nonce), then would it be simpler to just hard-code the nonces for the two files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point on using a crypto algorithm! This was copied from the vscode sample repo without me thinking too much...I would prefer not to use hard-coded nonces though, because it will pretty much defeat the entire purpose of using the nonce. Changed this to use crypto.randomBytes.

@shenglol
Copy link
Contributor Author

shenglol commented Apr 27, 2021

You're definitely going to need to add a number of end-to-end tests for this functionality - including the client-side stuff like opening/closing panes etc.

Yeah, for sure. I've been trying to figure how to test webviews and I made some progress. I'll add end-to-end tests to this PR.

Update: added e2e tests.


- name: Run snapshot tests
run: npm run test:snapshot
working-directory: ./src/vscode-bicep
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reorganized the build steps now that we get rid of tsc for unit tests and snapshot tests (they should happen before bundling).

@majastrz
Copy link
Member

majastrz commented May 7, 2021

When you save an untitled file when the visualizer is active, is it possible to also rename the active visualizer tab? Right now it seems to be creating a new empty untitled file and leaving repointing the visualizer to that file, which clears the view.

Copy link
Member

@majastrz majastrz left a comment

Choose a reason for hiding this comment

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

:shipit:

@shenglol
Copy link
Contributor Author

shenglol commented May 7, 2021

When you save an untitled file when the visualizer is active, is it possible to also rename the active visualizer tab? Right now it seems to be creating a new empty untitled file and leaving repointing the visualizer to that file, which clears the view.

Good catch! It is possible, the built-in markdown preview extension does that, and I'll need to check their source code to see how that's implemented. As this PR is getting a bit overwhelming, I'll send separate PRs to implement the feature along with some other changes I want to make:

  • Add a light theme and high contrast theme and settings to choose a theme. By default it should match the current VSCode theme category (dark, light, or high contrast).
  • Update the fonts used in the webview to match what VSCode uses and (potentially) add settings that allow users to override them.
  • Double-clicking a resource/module node to reveal the corresponding declaration in the source code.
  • Add controls to refresh, zoom in, zoom out the graph, etc.

@shenglol shenglol enabled auto-merge (squash) May 7, 2021 02:08
@shenglol shenglol merged commit 3acf968 into main May 7, 2021
@shenglol shenglol deleted the shenglol/bicep-visualizer branch May 7, 2021 02:19
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.

We need a visualizer for Bicep Output the dependency graph of produced template
4 participants