dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
34.79k stars 9.83k forks source link

Enable integration with 3rd party assets management solutions #38445

Open mkArtakMSFT opened 2 years ago

mkArtakMSFT commented 2 years ago

Consider productionizing Microsoft.AspNetCore.ClientAssets package

javiercn commented 2 years ago

Here is a draft of the design:

Productionizing the client assets package

There are several challenges integrating third-party tools that produce or transform web assets as part of the build process:

Goals

Out of scope

Technical design

We want to offer a declarative model where customers can indicate what tool to run, the inputs and the outputs and we take care of the rest.

We will likely need a couple of "phases" or extensibility points in the pipeline.

One initial phase to ensure that any third-party dependency toolchain (like npm or yarn) had a chance to run.

A second phase where we invoke whatever tool or script we are given.

Ideally, we want this to be a collaborative process where multiple tools can be added provided they don't depend on each other.

That allows packages to include a command for their toolchain as long as it doesn't depend on the outputs of any other tools.

One way we can make this work is by defining the invocations inside an item group as follows:

<ItemGroup>
  <ClientAssetTool Include="InstallNpmDependencies">
    <Stage>Dependencies</Stage>
    <Command>npm install</Command>
    <Directory>assets</Directory>
    <Inputs>assets\package.json;assets\package-lock.json</Inputs>
    <ExcludeInputs></ExcludeInputs>
    <Outputs>assets\node_modules\package-lock.json</Outputs>
    <ExcludeOutputs></ExcludeOutputs>
  </ClientAssetTool>

  <ClientAssetTool Include="RunNpmBuildScript">
    <Stage>Generation</Stage>
    <Command>npm run build:$(Configuration)</Command>
    <Directory>assets</Directory>
    <Inputs>assets\**</Inputs>
    <ExcludeInputs></ExcludeInputs>
    <Outputs>*.js</Outputs>
    <ExcludeOutputs>*.map.js</ExcludeOutputs>
  </ClientAssetTool>
</ItemGroup>

The metadata included in the item group defines all the aspects of how and when the tool needs to run:

With this metadata we will do as follows:

Notes on testing

The biggest part in this aspect is the incrementalism. Some scenarios that I can enumerate:

Questions

MarkStega commented 2 years ago

This is close to what I would like to see but I believe that there needs to be a way to have multiple sets of input & output specified. For example, I build both js & css; If a js source file changes I don't want to build css. As long as I can have multiple sections with generate and different commands/inputs/outputs I'd be covered

javiercn commented 2 years ago

@MarkStega this design currently contemplates that.

You will create two separate item group items each one specifying a different command and a different set of inputs/outputs, one for your CSS and one for your JS

MarkStega commented 2 years ago

@TanayParikh I am very interested in using this feature so I will volunteer to test off of nightly builds when you get to that point.

mkArtakMSFT commented 1 year ago

@TanayParikh please make sure this is also covered as part of the work: https://github.com/dotnet/aspnetcore/issues/42472

MariovanZeist commented 1 year ago

@TanayParikh I have been using this package for a while now. (Well a slightly modified version of this library to be fair) The only problem I had with it was that when the path would include spaces, the output would not be generated I created a PR to fix the issue https://github.com/aspnet/AspLabs/pull/405, (And I have been using that modified version myself) but I must admit I am not a node nor an MSBuild specialist, so my fix could be wrong.

Do you have any insights into this?

javiercn commented 1 year ago

@MariovanZeist we do not plan to move forward with the experimental package (as we are planning to integrate the functionality directly into the SDK). Being an experimental package, it was expected to have bugs. We plan to address things like this as part of moving the functionality to the SDK repo, as we have there the infrastructure to test it properly.

That said, the new functionality will give you the ability to define the command yourself, so you'll be responsible for things being quoted appropriately.

We might define some out of the box conventions either on the SDK or inside a new package to further streamline the scenario when following the conventions (so that basically you put assets in a specific folder, and everything magically works after that). In that case we would be taking care of the quoting.

MariovanZeist commented 1 year ago

@javiercn Thanks for clearing it up, I was indeed under the mistaken assumption that this particular package would be used. I prefer it being part of the SDK anyway, so thanks for the info.

javiercn commented 1 year ago

@MariovanZeist yeah, the idea is that we make this process declarative, you get to say what commands to run, what inputs and outputs and we take care of making sure that they end up in the right place inside the wwwroot folder and that things are done incrementally (provided you correctly specify inputs and outputs).

It is much easier for us to test those things in the SDK repo because we have infrastructure that can create a project and run through different scenarios to make sure that stuff works, and it does not regress over time when changes are made.

jeffputz commented 1 year ago

I want to reiterate the desire from #40572 about how there is a production need as well as a development need. Even the nomenclature of "asset" implies that Typescript is not code. If I were to simplify the need it looks like this:

ghost commented 9 months ago

Thanks for contacting us.

We're moving this issue to the .NET 9 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

ghost commented 6 months ago

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.