reimplement button-to-open-in-new-tab + custom webview #133 (monorepo)

[kevinramharak]

Reimplemented #133 but with the monorepo setup. Used a new branch to avoid having to solve the merge errors that would come from trying to merge main back into that branch.

Whats left:

• Use of https://github.com/yoavbls/vscode-shiki-bridge instead of custom CSS (if applicable)
• Injecting the vscode-codeicon icon font (example)
• Removing the Open in new Tab link in the Markdown preview (its there, just not shown because the icons don't show up)
• Maybe take another stab at injecting the type grammar this is not needed if we use vscode-shiki-bridge

• I'm not sure why, but if a panel is opened and I click on another one, it stays on the previous one.
  Is there something to do about it? (copied from here)

We can also extend the markdown-it as @MichaelDimmitt also mentioned. Some references are:

• vscode docs
• vscode-markdown-extended
• markdown-it

Some of the bugs/limitations can be discovered in how the markdown preview is implemented at markdown-language-features

This does indicate that a custom webview could be a better option, as it would be a lot less limiting. As for placement, a custom webview can be rendered pretty much everywhere, as an editor tab or part of the activity bar, so we could add configuration to let the user choose where to render it, with a sensible default.

[kevinramharak]

@yoavbls I ported the changes from web-preview-poc to this branch and included use of vscode-shiki-bridge.
It's messy and buggy, some weird build behaviour causing jsonc-parser to sometimes break at runtime when it is internally requireing some file.
A bunch of bugs / missing features of vscode-shiki-bridge that I added comments for (will convert them to proper issues later).

But it does work:

I think this is enough of poc to confirm this works and is a better approach. Maybe we start with implementing it with the default dark/light highlighters dark-plus and light-plus as they are bundled with shiki. vscode-shiki-bridge just needs some work to properly resolve and fetch language grammars and theme files, and when that works properly the rest of the feature will have the correct highlighting and theme out of the box.

[yoavbls]

wowwww I'm so excited to see that this POC is working!
I understand that shiki-bridge is quite broken, I'll try to fix things there over the weekend and update this PR.
If it seems like the web view is working, can we remove the markdown parts and checks?

[kevinramharak]

@yoavbls I think we still need these parts:

• The formatter still needs to embed the codeicon button that triggers the command
• A part that registers the TextDocumentContentProvider API for our custom URI scheme and returns the formatted message as markdown contents.
• A part that registers the command to open in a new tab, which will call vscode.window.createWebviewPanel, fetch the contents through our custom text document content provider and embed it in the webview HTML.

The main change is that instead of executing the markdown.showPreview command, we will execute the tsPrettyErrors.openMarkdownPreview command.

We could embed it all in the command that creates the webview, but having the custom text document content provider forces a separation of concerns. Which I think is a good thing here.

During prototyping of this and the l10n draft I ran into the issue that passing the diagnostic as a string and performing string replacements makes it quite difficult to reason about the state of the string and what is and isn't part of it. For example removing the Open in new Tab link in the formatted message, is going to require another regex to find and replace it.

I also suspect that the heavy use of regexes is part of the performance issues some people experience (although pretty sure it's also caused by huge projects with lots of files).

So if there is a way to implement a more abstract representation of the (formatted) diagnostic, it could help, maybe not so much in this issue, but for future features and changes.

Something more like:

type FormattedDiagnosticPartType = 'text' | 'codeblock' | 'icon' | 'link';

type FormattedDiagnosticPart = { type: FormattedDiagnosticPartType, text: string }

interface FormattedDiagnostic {
  parts: FormattedDiagnosticPart[];
  code: number;
  location: Uri;
  range: Range;
}

I don't think we need it in this implementation, but keeping it in mind while implementing could really help to figure out if we need it and how a consuming API would use a data structure instead of a string.

[kevinramharak]

@yoavbls @MichaelDimmitt

We are getting close:

The old implementation was pretty easy to stitch to the new one, the hardest parts were figuring out CSP and the way we link to a symbol had to change. It's a bit smarter now as well, so it wont open links in the preview in the same active panel.

Code needs a bit of cleanup, but the feature is very close to complete.

[yoavbls]

I didn't review everything yet but I tested it out and it's amazing! I could not wait to merge it 😀
The main things should be done before:

1. Upgrade vscode-shiki-bridge
2. Use the user's theme for the code blocks
3. Get rid of the previous logic meant for the markdown-based solution instead of the webview.
4. Make the panel update instead of opening a new one
5. Make the persistent panel on the explorer section update when clicking on a new error. I really liked this one

Thank you for all of this work
@kevinramharak @MichaelDimmitt

[kevinramharak]

@yoavbls I think this is almost done now. Just needs to use the vscode-shiki-bridge update to use the user theme and languages to highlight the code blocks.

The custom markdown rendering was a bit buggy and generating incorrect HTML which messed up the preview. I ended up trying to integrate markdown-exit as it seemed to integrate with shiki nicely. Besides 1 annoying issue of the markdown renderer wrapping fenced code blocks, it worked out pretty well:

I think having this setup works well, as when I use my laptop with a smaller screen, the preview panel is often not large enough, so being able to open any error in a new tab is very helpful.

The view in the explorer panel now follows the cursor/selection, and feels pretty good. I initially cleared the panel when moving the cursor to a selection without an error, but that can be annoying, so now it just shows the last error for the code you selected.

This solves a lot of issues with the hover not being copiable (#155), as both the new tab preview and the explorer panel allow for selecting and copying text. Additionally the new copy button #156 allows for copying the whole error message, and each code block now has a copy button. I added a notification message to give some user feedback that the click on the button actually did something.

If you can take a look at yoavbls/vscode-shiki-bridge#2 and create a release for it, then all that remains is update the highlighting code to use the user theme, and this should be ready to go.

[kevinramharak]

@yoavbls You happened to push the update, so I stitched it together. I tried it with my own theme and another theme. I think this is now complete.

There could be some QOL things like loading new themes when the theme changes, but it's nothing a restart or reload of VSCode won't fix.

[yoavbls]

Great! yes It's fine if a restart is required in case there is a new extension or theme.
So i'm checking it and let's merge 💃🏼💃🏼

[kevinramharak]

There is indeed an issue for packaging/publishing the .vsix:

ERROR  SVGs are restricted in README.md; please use other file image formats, such as PNG: https://custom-icon-badges.demolab.com/badge/Visual Studio Code-0078d7.svg?logo=vsc&logoColor=white

I fixed it with 6700a52. Unfortunatly the whitelisted badge providers require to embed the vscode extension icon as a base64 embedded png. But using a markdown reference, its not to ugly. I had to use the flat white icon for cursor instead of the original one, as its the only one imgshields provides.

This allows for building the .vsix properly. I will be using the custom .vsix for a bit to see if it works correctly. @yoavbls We could also release 0.7.0 as a prerelease just for a bit to allow people to use it and see if there any major bugs. And release it normally at 0.7.x afterwards.

---

I think the infinite loop on dev is related to mac OS specifics, I cannot reproduce it. Maybe check the git status to see if uncommited files are generated.

---

I fixed the copy type button, the implementation was a bit hacky dumping the content in an HTML attrivute, now it just copies the innerText from the <pre><code> tag, which should give the exact same result as selecting the text in a code block and copying it manually.

---

As for the type codeblocks not wrapping that is strange, I see the following:

Does it wrap in the hover prettified error? Can you try opening the developer tools from VS Code and see what the HTML is of those codeblocks? Anything in a pre should have the CSS property white-space: pre;, which should render any \n as an actual new line.

If needed we can use Shiki's output to add some CSS, as each line will be wrapped in a <span class="line".

[MichaelDimmitt]

Awesome, I will grab latest and test it out

[MichaelDimmitt]

Tested it out and looking good now!

[MichaelDimmitt]

Tip: tsconfig, "noErrorTruncation": true,

If you are getting output in one line but need to see it in what is depicted in the screenshots change your tsconfig to have a compilerOption "noErrorTruncation": true,

[MichaelDimmitt]

TLDR: Works, thanks!

[MichaelDimmitt]

Nice use of css variables here to get the vscode themes!
I think that is what is going on here alongside TmLanguage-Syntax-Highlighter
and these libraries: markdown-exit, shiki, and vscode-shiki-bridge

[MichaelDimmitt]

Read through the pr, functionality works on my end.

Local development setup was a little difficult but I got it figured out.

It looks like the only outstanding task is writing a script to move the codicon.css file out of nodemodules instead of keeping your own version? But probably out of scope of this pr.

Thanks for adding this! @kevinramharak and @yoavbls

[MichaelDimmitt]

Not sure if you want your contribution.md file to have developer setup instructions for getting debugging environment up and running. The dev setup has changed a bit since the mono repo update. But also this change might be out of scope of this pr.

[kevinramharak]

I thought there was an issue for this, but I cant find it. This is indeed on the todo list after the monorepo setup. I will do this next, as it is intended to easily setup and contribute to this repo

[MichaelDimmitt]

It looks like this file is the main change and uses markdown-exit, shiki, and vscode-shiki-bridge.

[kevinramharak]

@MichaelDimmitt That is very helpful. If you want, you can create a separate issue for it, but I will probably have some time this week to create and fix the CONTRIBUTING.md stuff.

[MichaelDimmitt]

Documented error and docs request in:
#158 and
#159

I'll remove those comments from this pr now.

[yoavbls]

It looks good to me, do you think we can merge that?
@kevinramharak @MichaelDimmitt
After that, I hope we'll be able to remove markdown-exit and rely only on WebView for it.
The second parameter of formatDiagnosticMessage can be used for that:

pretty-ts-errors/packages/formatter/src/formatDiagnosticMessage.ts

Line 5 in 31caba3

codeBlock: (code: string, language?: string, multiLine?: boolean) => string

WDYT?

[kevinramharak]

@yoavbls I think this is ready for release. I just added some minor tweaks to the CSS and UX.

I'm not sure how we can drop markdown-exit or a different markdown renderer, as we need something to render the markdown to the webiew.
I added it in b6a80b9 because the simpler custom one did not work very well.

We can try and instead of storing the markdown version of the error message, we store the fully HTML rendered one. But then the hover implementation might not work, as its sanitazation is quite strict.

EDIT:
I think I understand what you mean, we can definitely look into that. As it is indeed only the codeblocks that we really use markdown for. Its mainly the sanitazing the hover feature does that causes issues.

[yoavbls]

EDIT: I think I understand what you mean, we can definitely look into that. As it is indeed only the codeblocks that we really use markdown for. Its mainly the sanitazing the hover feature does that causes issues.

Yes, exactly, now we have a function that renders code blocks to HTML, so we can render a different output for webviews, without markdown
But we'll address it after it, I'm taking a final look for merge 🙂

[yoavbls]

I think I'll try it until tomorrow and release after Christmas Eve 🎅🏼🎄
@kevinramharak @MichaelDimmitt Thank you guys, you're welcome to try it too:
pretty-ts-errors-0.7.0.vsix.zip