microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
101.04k stars 12.49k forks source link

Add the Node that comes with Visual Studio as a NodePath fallback in the .targets file in the Microsoft.TypeScript.MSBuild Nuget package #38247

Open kelps opened 4 years ago

kelps commented 4 years ago

I am working on a new project using ASPNET Core 3.1 and I wanted to write our font-end code in TypeScript, but found a little problem: You can't use TypeScript and build the project using the dotnet CLI commands if you don't have a stand alone installation of Node. If you build using Visual Studio, it works ok because VS now comes with Node, but building in Azure Pipelines or local with the "dotnet build" command results in an error saying Node could not be found.

My project is referencing the Microsoft.TypeScript.MSBuild nuget package.

On March 20th 2020, I created a simple repro at https://github.com/kelps/TypeScriptBuildRepro and also submitted a VS feedback about this at https://developercommunity.visualstudio.com/content/problem/957892/typescript-files-do-not-build-using-dotnet-cli-com.html.

@timheuer helped me diagnose it and the problem was that Node needed to be in the Path to work. I didn't like that because Visual Studio already comes with Node and I shouldn't need to have another separate Node install to run this. @madskristensen's BuildWebCompiler Nuget package also uses Node to compile .less/.sass/... files and it just works, without a stand alone Node install.

After a long time testing and researching I was able to create a very simple work around. I added a msbuild Task to my .csproj file that runs before the TypeScript compilation and sets the NodePath to the one that comes with VS if it is still empty. When it worked, I removed that code from my .csproj file and placed it in the Directory.Build.targets file in the solution directory.

My suggestion is to add some form of that code to the .targets file in the Microsoft.TypeScript.MSBuild Nuget package.

The master in my repo now has the code that works. To see the build failling, just rename the Directory.Build.targets file and try to build the solution with "dotnet build" in a computer without a stand alone Node installation (or at least without Node in the %path%).

It is a simple "fix" that provides a good fallback for TypeScript compilation with ASPNET Core on Windows. I am sure my code can be improved, but it is a good start. It uses vswhere to find the VS Node path.

For reference, here is the code in my .targets file.

<Project ToolsVersion="15.0">
  <!--https://stackoverflow.com/questions/31664834/customize-system-environment-variable-path-for-msbuild-exec-task#31670922-->
  <!--http://blog.jdhardy.ca/2011/12/setting-environment-variables-for.html-->
  <!--bat script for findind the Visual Studio instalation path for Node and setting it in the appropriate variable before building TypeScript files-->
  <PropertyGroup>
    <SetNodePath>
    <![CDATA[
    @setlocal enabledelayedexpansion
    @for /f "usebackq tokens=*" %%i in (`"%ProgramFiles(x86)%\Microsoft Visual Studio\Installer\vswhere" -latest -find **\node.exe`) do (@set nodePath=%%~dpi)
    @echo %NodePath%
    ]]></SetNodePath>
  </PropertyGroup>

  <!--Target that runs the script above if the node path isn't set yet and it is running on Windows-->
  <Target Name="FindAndSetNodePath" BeforeTargets="PreComputeCompileTypeScriptWithTSConfig" Condition="'$(NodePath)' == '' AND '$(OS)' == 'Windows_NT' AND exists('$(MSBuildProgramFiles32)\Microsoft Visual Studio\Installer\vswhere.exe')">
    <Exec Command="$(SetNodePath)" ConsoleToMSBuild="true">
      <Output TaskParameter="ConsoleOutput" PropertyName="NodePath"/>
    </Exec>
    <Message ContinueOnError="true" Text="NodePath: $(NodePath)" Importance="high" />
  </Target>
</Project>
minestarks commented 4 years ago

Thanks @kelps , while I think your workaround is brilliant, I'd like to be very careful about making this a part of the TypeScript package. The reason being that in general we want our build tools to be as deterministic as possible, and using vswhere (or an equivalent) will make the package even more environment-dependent. This is something we generally want to avoid -- the obvious reason being the one you've already called out: it might work in your local environment, but not work when you deploy it to something like a CI machine. Or your build may subtly change without you noticing when you install different versions of VS side by side, or you uninstall VS. This solution may solve this specific scenario for you, but I'm not convinced it applies to most users.

While using %PATH% is also obviously very environment-dependent, the alternatives were "build doesn't work outside VS at all" and "bundle Node.exe in the Nuget package" and we found this an acceptable option.

kelps commented 4 years ago

I do understand your point, but I still think this is a good fallback to put in the package, because it already has other 2 similar fallbacks in the .targets file that point to other (maybe legacy) paths. I was only able to make my workaround succeed because I saw what the package's .targets file was already doing.

As I see it, this will only increase the cases where the Nuget package just works (becoming a little more deterministic). It already has a check if vswhere exists and only runs if that is true and the NodePath is still empty, so, in a case where vswhere isn't available, the build will simply behave the same way it does today. But when we do have vswhere and Node isn't in the Path, it gives an extra chance for the build to work.

I do hope this improvement (or something similar) would be considered.

Thanks

Griffork commented 3 years ago

Note: this does not address the original problem, but more offers a (probably undesirable) workaround:

We use a .njsproj for our front-end development and it builds just fine on Azure Pipelines when using the Visual Studio Build command (presumably because it's using JSTools and not a nuget package?). Do you have a separate project for your front end, or is it all in the same .net project as the backend?

For reference, we're also using BuildWebCompiler with no issues.


Because I'm bad at checking github for issue updates, and in case you decide to try using a .njsproj here are the instructions for installing the BuildWebCompiler nuget package into a .njsproj:

Add the following lines to the end of your .njsproj file:

  <Import Project=".\packages\BuildWebCompiler.1.12.394\build\BuildWebCompiler.targets" Condition="Exists('.\packages\BuildWebCompiler.1.12.394\build\BuildWebCompiler.targets')" />
  <Target Name="EnsureNuGetPackageBuildImports" BeforeTargets="PrepareForBuild">
    <PropertyGroup>
      <ErrorText>This project references NuGet package(s) that are missing on this computer. Use NuGet Package Restore to download them.  For more information, see http://go.microsoft.com/fwlink/?LinkID=322105. The missing file is {0}.</ErrorText>
    </PropertyGroup>
    <Error Condition="!Exists('.\packages\BuildWebCompiler.1.12.394\build\BuildWebCompiler.targets')" Text="$([System.String]::Format('$(ErrorText)', '..\packages\BuildWebCompiler.1.12.394\build\BuildWebCompiler.targets'))" />
  </Target>

(replace the version number with the version of BuildWebCompiler you actually want)

Add a packages.config file to the directory of your project with the following:

<?xml version="1.0" encoding="utf-8"?>
<packages>
  <package id="BuildWebCompiler" version="1.12.394" targetFramework="net40" />
</packages>

(again change the version number as appropriate)

On Azure Pipeline add a NuGet restore command version 2.* with the path set to your project's packages.config (e.g. Client\packages.config).

On developer machines you will need to install it manually because .njsproj doesn't support .nuget packages by default. I've committed a script file to my repo that does the following:


I hope any of this is helpful!