Add preview Razor support #2554

SteveSandersonMS · 2018-09-24T10:24:32Z

As discussed previously, this adds Razor support behind a new omnisharp.preview options flag.

It works by downloading a platform-specific Razor Language Server executable (equivalent to the OmniSharp-Roslyn language server) and then running some VSCode-side logic that knows how to launch and interact with it.

SteveSandersonMS · 2018-09-24T10:27:22Z

package.json

@@ -73,6 +73,7 @@
    "http-proxy-agent": "2.1.0",
    "https-proxy-agent": "2.2.1",
    "jsonc-parser": "1.0.0",
+    "microsoft.aspnetcore.razor.vscode": "https://razorvscodetest.blob.core.windows.net/npm/microsoft.aspnetcore.razor.vscode-1.0.0-alpha1-20180924.3.tgz",


We're consuming this via URL, rather than from npmjs.org, because there's no reason why any other code should want to take a dependency on it. There's just no reason for this to package to be on npmjs.org.

SteveSandersonMS · 2018-09-24T10:28:04Z

package.json

@@ -84,7 +85,7 @@
    "tmp": "0.0.33",
    "vscode-debugprotocol": "1.6.1",
    "vscode-extension-telemetry": "0.0.15",
-    "yauzl": "2.9.1"
+    "yauzl": "2.10.0"


Needed to get the newer version of yauzl for compatibility with the Razor Language Server zipfiles.

SteveSandersonMS · 2018-09-24T10:28:52Z

package.json

+    },
+    {
+      "description": "Razor Language Server (Windows / x64)",
+      "url": "https://razorvscodetest.blob.core.windows.net/languageserver/RazorLanguageServer-win-x64-1.0.0-alpha1-20180924.3.zip",


These URLs will need to change before this ships. We want the downloads to come from a particular CDN which we're still getting access to.

SteveSandersonMS · 2018-09-24T10:31:13Z

package.json

+      {
+        "id": "aspnetcorerazor",
+        "extensions": [
+          ".cshtml"


We're overriding VS Code's built-in .cshtml handling by mapping it to a different language ID. This separate aspnetcorerazor language ID lets us control exactly what behaviors are applied to it, so we can give a better experience (e.g., around HTML completion) that is more deeply aware of Razor language semantics.

Shoot, just thought of one side effect of this. If the omnisharp.preview flag is false but we're still overriding the normal Razor language id I think this will nuke some of the default .cshtml editing experiences (no HTML completion).

Does VSCode allow us to toggle the language on/off?

That's a great point. Unfortunately I don't have any easy fix in mind for it.

I checked, and as expected, having omnisharp.preview set to false means that .cshtml files:

Still get correct colorization, since that's defined declaratively

No longer get completions for HTML/JS

No longer get automatic closing-tag insertion for HTML

As such it's still a working experience, but it is degraded in functionality compared with the default.

Possible ways forwards:

Decide that Razor support is looking good already and turn it on by default in OmniSharp-VSCode

Or, accept the reduced functionality for .cshtml when preview is off (not great, but consider that the default .cshtml handling is very poor anyway, e.g., because of the List<T> stuff mentioned below)

Or, move Razor support out into a completely separate extension (not the installation experience we wanted)

Or, ask the VS Code team to provide some feature by which an extension can respond to the onLanguage:(languageId) event and programmatically decline the event, causing VS Code to drop that language entry from its config and proceed to activate the next extension that matches the selector. Or any other design the VS Code team prefers.

Then we wait for it to be implemented and ship

Or, try to implement our .cshtml support without defining a new language ID.

Unfortunately that is what we tried originally and it didn't work well because VS Code consulted both our extension and the built-in Razor feature for completions (etc), leading to duplication in the UI and incorrect awareness of what should show up in HTML blocks vs C# blocks etc. For example, if you try to type List<string> in a C# context, the built-in HTML tag completion kicks in and inserts </string> right after, which is insane.

At the very least, going this route is likely to involve some work with and by the VS Code folks to find some new way of suppressing the default behaviors programmatically.

Or, we create some new default behavior (when preview is false) whereby we do activate the Razor code paths, but they only do what they need to recreate the default functionality. That is, they still do HTML tag closing and HTML/JS completions etc., but they just skip launching the Razor Language Server and treat the entire document as if it was HTML.

Assuming you don't think things are already good enough that you just want to turn it on, have you thought about maybe having a 'razor' branch in this repo where you can release beta versions of this extension with the razor support enabled, and folks who want to try it can install this new extension rather than toggling an option?

@SteveSandersonMS lets talk more offline in our sync up today.

@gregg-miskelly we have. Our goals are to get the extension into as many peoples hands as possible. Forcing users to go and download a pre-release version would be an experience blocker and would prohibit us from getting enough feedback to make educated decisions about future releases.

codecov · 2018-09-24T10:33:41Z

Codecov Report

Merging #2554 into master will increase coverage by 0.27%.
The diff coverage is 76.92%.

@@            Coverage Diff             @@
##           master    #2554      +/-   ##
==========================================
+ Coverage   65.14%   65.41%   +0.27%     
==========================================
  Files          97       98       +1     
  Lines        4226     4248      +22     
  Branches      605      610       +5     
==========================================
+ Hits         2753     2779      +26     
+ Misses       1301     1295       -6     
- Partials      172      174       +2

Flag	Coverage Δ
#integration	`53.47% <76.92%> (+0.5%)`	⬆️
#unit	`84.73% <75%> (-0.04%)`	⬇️

Impacted Files	Coverage Δ
src/omnisharp/options.ts	`100% <100%> (ø)`	⬆️
src/main.ts	`92.66% <66.66%> (-0.74%)`	⬇️
src/omnisharp/server.ts	`71.37% <71.42%> (-0.3%)`	⬇️
src/razor/razor.ts	`75% <75%> (ø)`
src/omnisharp/requestQueue.ts	`81.81% <0%> (-3.04%)`	⬇️
src/features/diagnosticsProvider.ts	`76.81% <0%> (+1.44%)`	⬆️
src/features/completionItemProvider.ts	`87.87% <0%> (+9.09%)`	⬆️
src/features/documentation.ts	`47.61% <0%> (+9.52%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 984b2f8...f33a7b5. Read the comment docs.

SteveSandersonMS · 2018-09-24T10:33:55Z

src/razor/razor.ts

+/*---------------------------------------------------------------------------------------------
+ *  Copyright (c) Microsoft Corporation. All rights reserved.
+ *  Licensed under the MIT License. See License.txt in the project root for license information.
+ *--------------------------------------------------------------------------------------------*/


The logic in this file uses vscode APIs directly, not going through vscodeAdapter, for reasons we've discussed previously. The microsoft.aspnetcore.razor.vscode NPM package uses vscode directly extensively, so nothing extra would be gained by putting this tiny bit of wrapper logic through additional abstraction.

Later on we do want to move microsoft.aspnetcore.razor.vscode entirely to share OmniSharp-VSCode's vscodeAdapter pattern for testability, but that's a bit further out.

SteveSandersonMS · 2018-09-24T10:34:41Z

src/razor/syntaxes/aspnetcorerazor.tmLanguage.json

+	"scopeName": "text.aspnetcorerazor",
+	"patterns": [
+		{
+			"include": "text.html.cshtml"


This causes the aspnetcorerazor language to retain VSCode's built-in colorization, but we override everything else.

SteveSandersonMS · 2018-09-24T16:43:53Z

package.json

+      "editor/title": [
+        {
+          "command": "extension.showRazorCSharpWindow",
+          "when": "resourceLangId == razor"


Note: I need to change the language ID here and below

NTaylorMullen · 2018-09-24T17:06:33Z

package.json

@@ -602,6 +653,29 @@
          "type": "boolean",
          "default": false,
          "description": "Specifies whether notifications should be shown if OmniSharp encounters warnings or errors loading a project. Note that these warnings/errors are always emitted to the OmniSharp log"
+        },
+        "omnisharp.preview": {


This doesn't already exist? @rchande / @akshita31 / @TheRealPiotrP I thought you guys already had an omnisharp.preview flag that we were sitting behind?

@NTaylorMullen We didn't have a flag like that before.

Ah, alright. Well 👍

NTaylorMullen · 2018-09-24T17:07:27Z

package.json

+          "default": false,
+          "description": "Specifies whether to enable OmniSharp preview features."
+        },
+        "razor.languageServer.path": {


Since this is really pointing to the directory we should consider changing this to razor.languageServer.directory

rchande · 2018-09-24T17:09:41Z

@SteveSandersonMS I pulled this down and played around with it. Seems to work--very cool!

filipw · 2018-09-24T17:12:24Z

what is RazorLanguageServer? is it available somewhere on Github?

NTaylorMullen · 2018-09-24T17:12:41Z

what is RazorLanguageServer? is it available somewhere on Github?

RazorLanguageServer is the brains behind getting a fluent Razor experience in VSCode. Working on getting the repo open source 😉

akshita31 · 2018-09-24T18:21:20Z

package.json

+        "razor.languageServer.trace": {
+          "type": "string",
+          "default": "Messages",
+          "description": "Specifies whether to output all messages [Verbose], some messages [Messages] or not at all [Off]."


Are these the only possible inputs the user must specify. If so, we can use an enum type here, similar to https://github.com/OmniSharp/omnisharp-vscode/blob/master/package.json#L528

Good tip - thanks. Done.

akshita31 · 2018-09-24T18:52:02Z

Also, I am curious to understand the logging scenario here. Should we have some log that says "Razor activated"? If we get some issues after the release, what is the best way to understand that the razor server is activated(other than asking the users if the preview flag is true)?
cc @rchande

NTaylorMullen · 2018-09-24T19:15:35Z

package.json

+        },
+        "razor.languageServer.trace": {
+          "type": "string",
+          "default": "Messages",


Lets actually default to Off. In a coming PR I always log at least something to indicate that Razor is enabled and i'd rather do less work in the scenarios of logging.

NTaylorMullen · 2018-09-24T19:16:38Z

Also, I am curious to understand the logging scenario here. Should we have some log that says "Razor activated"? If we get some issues after the release, what is the best way to understand that the razor server is activated(other than asking the users if the preview flag is true)?

We'll have a Razor log output window that will indicate that. If you guys feel that there should also be a log in the Omnisharp output window we can definitely add one there as well.

rchande · 2018-09-24T19:19:46Z

@NTaylorMullen Let's keep the logs separate for now, unless we find that there are a lot of issues that require closely correlating timestamps between both logs.

NTaylorMullen · 2018-09-25T00:58:52Z

src/main.ts

@@ -134,6 +135,10 @@ export async function activate(context: vscode.ExtensionContext): Promise<CSharp
        coreClrDebugPromise = coreclrdebug.activate(extension, context, platformInfo, eventStream);
    }

+    if (optionProvider.GetLatestOptions().preview) {
+        activateRazorExtension(context);


Should you be awaiting this?

Good point, yes, that's better.

NTaylorMullen

Looks good, we can always push additional changes.

NTaylorMullen · 2018-09-26T23:09:39Z

You'll actually also need to update this section in packages.json:

      "type": "coreclr",
        "label": ".NET Core",
        "enableBreakpointsFor": {
          "languageIds": [
            "csharp",
            "razor"
          ]
        },

"type": "clr",
        "label": ".NET",
        "enableBreakpointsFor": {
          "languageIds": [
            "csharp",
            "razor"
          ]
        },

rchande · 2018-09-27T16:07:17Z

@SteveSandersonMS @NTaylorMullen How hard will it be for you to react to #2552 when it merged? Would we be better off waiting on that PR?

NTaylorMullen · 2018-09-27T17:29:45Z

@SteveSandersonMS @NTaylorMullen How hard will it be for you to react to #2552 when it merged? Would we be better off waiting on that PR?

Already reacted. We'll just need to update the npm package / Language Server versions that are pulled down. I don't believe this PR currently is targeting the npm/language server bits with the reaction but either way it's super easy to add; let us know 😉

mattwelke · 2018-10-01T14:18:25Z

Came here from #168. I notice one of the comments above states that making the Razor support available as a separate extension would solve some of these early issues but wouldn't be the installation experienced that was originally wanted. I just wanted to chime in and mention that I think it is a good idea to make it a different extension. Not all of the ASP.NET Core projects I develop in Visual Studio Code use Razor. In fact, some people may create C# projects that have nothing to do with the web.

NTaylorMullen · 2018-10-01T18:11:15Z

@Welkie I totally get your concern. Even though the Razor experience is planned to be included as part of the C# extension it will only operate on Razor files and should not have any perf-impact to that of C# files.

NTaylorMullen · 2018-10-01T18:11:40Z

@SteveSandersonMS is this good to merge now?

akshita31 · 2018-10-02T03:53:42Z

src/omnisharp/server.ts

@@ -304,6 +304,15 @@ export class OmniSharpServer {
            '--loglevel', options.loggingLevel
        ];

+        if (!options.razorDisabled) {
+            const razorPluginPath = path.join(
+                utils.getExtensionPath(),


server takes an extension path in the constructor. We should use that here

Good plan. Done.

NTaylorMullen · 2018-10-02T06:54:37Z

would still lead to degraded experience for the html completions right

Yup that's correct. It's not ideal but the primary purpose of the razor.disabled flag exists as a backup in-case we explode something unintentionally and break another piece of VSCode unintentionally.

why do we feel that is better than having razor disabled by default

Even if Razor is disabled by default you still wouldn't get the vanilla Razor experience that exists today. Sadly this is all a restriction in how VSCode extensions can extend/replace languages 😢

…option to turn them off

SteveSandersonMS · 2018-10-02T09:21:13Z

The remaining Travis CI failure appears unrelated to this PR. It happens if you run the integration tests on master currently too.

rchande · 2018-10-02T15:03:30Z

@SteveSandersonMS I'm looking into your CI failure now.

rchande · 2018-10-02T16:12:10Z

@NTaylorMullen @SteveSandersonMS is this ready to merge?

SteveSandersonMS · 2018-10-02T16:14:01Z

Yes, and thanks for resolving the CI issue!

…initial-razor-support" This reverts commit 44536b3, reversing changes made to 984b2f8.

Revert "Merge pull request #2554 from SteveSandersonMS/stevesa/initia…

SteveSandersonMS commented Sep 24, 2018

View reviewed changes

NTaylorMullen reviewed Sep 24, 2018

View reviewed changes

akshita31 reviewed Sep 24, 2018

View reviewed changes

NTaylorMullen reviewed Sep 24, 2018

View reviewed changes

NTaylorMullen reviewed Sep 25, 2018

View reviewed changes

NTaylorMullen approved these changes Sep 26, 2018

View reviewed changes

SIkebe mentioned this pull request Sep 27, 2018

Is it possibile to implement Razor intellisense / syntax highlighting out of Visual Studio? aspnet/Razor#2615

Closed

SIkebe mentioned this pull request Oct 1, 2018

No intellisense available for razor #168

Closed

SteveSandersonMS force-pushed the stevesa/initial-razor-support branch from 1c7b41c to dad83f2 Compare October 1, 2018 09:19

rchande approved these changes Oct 1, 2018

View reviewed changes

akshita31 reviewed Oct 2, 2018

View reviewed changes

SteveSandersonMS added 15 commits October 2, 2018 09:31

Add preview Razor support

bd5c4c5

Update to newer dependency URLs on VS CDN

84382bb

CR: Update language ID

4375e44

CR: Add 'await'

da2c32e

CR: Change razor.languageServer.path to razor.languageServer.directory

8a382cc

CR: Add enum for razor.languageServer.trace option

367f58e

Also activate on Razor files

2a8b7cc

Update lockfile

f7a2ccb

CR: Update breakpoints config

c1c4b2b

Update Razor binaries dependencies and update to exe name 'rzls'

df93bdd

Update to newer Razor activation signature

8ca1228

CR: Enable Razor features by default; define 'razor.disabled' config …

c0c1ac6

…option to turn them off

Conditionally load OmniSharp server plugin

0f30bbd

CR: Pass extensionPath into razor.ts

f78333d

CR: Use extensionPath from ctor in server.ts

1bc7abd

SteveSandersonMS force-pushed the stevesa/initial-razor-support branch from 2139363 to 1bc7abd Compare October 2, 2018 08:42

SteveSandersonMS added 2 commits October 2, 2018 09:45

Conditionally load OmniSharp server plugin

0b79a73

Fix tests

0fe633d

Merge branch 'master' into stevesa/initial-razor-support

f33a7b5

rchande merged commit 44536b3 into dotnet:master Oct 2, 2018

rchande pushed a commit to rchande/omnisharp-vscode that referenced this pull request Oct 2, 2018

Revert "Merge pull request dotnet#2554 from SteveSandersonMS/stevesa/…

b45e819

…initial-razor-support" This reverts commit 44536b3, reversing changes made to 984b2f8.

rchande pushed a commit that referenced this pull request Oct 2, 2018

Merge pull request #2569 from rchande/revertRazorPR

5c44344

Revert "Merge pull request #2554 from SteveSandersonMS/stevesa/initia…

SteveSandersonMS deleted the stevesa/initial-razor-support branch October 4, 2018 10:03

SteveSandersonMS mentioned this pull request Oct 4, 2018

Initial Razor support #2576

Merged

Add preview Razor support #2554

Add preview Razor support #2554

Conversation

SteveSandersonMS commented Sep 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveSandersonMS Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 24, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveSandersonMS Sep 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rchande commented Sep 24, 2018

filipw commented Sep 24, 2018

NTaylorMullen commented Sep 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akshita31 commented Sep 24, 2018

NTaylorMullen Sep 24, 2018 • edited Loading

Choose a reason for hiding this comment

NTaylorMullen commented Sep 24, 2018

rchande commented Sep 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NTaylorMullen left a comment

Choose a reason for hiding this comment

NTaylorMullen commented Sep 26, 2018 • edited Loading

rchande commented Sep 27, 2018

NTaylorMullen commented Sep 27, 2018

mattwelke commented Oct 1, 2018

NTaylorMullen commented Oct 1, 2018

NTaylorMullen commented Oct 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NTaylorMullen commented Oct 2, 2018

SteveSandersonMS commented Oct 2, 2018

rchande commented Oct 2, 2018

rchande commented Oct 2, 2018

SteveSandersonMS commented Oct 2, 2018

SteveSandersonMS Sep 26, 2018 •

edited

Loading

codecov bot commented Sep 24, 2018 •

edited

Loading

SteveSandersonMS Sep 24, 2018 •

edited

Loading

NTaylorMullen commented Sep 24, 2018 •

edited

Loading

NTaylorMullen Sep 24, 2018 •

edited

Loading

NTaylorMullen commented Sep 26, 2018 •

edited

Loading