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

Configurable wwwroot for StaticWebAssets #34178

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Conversation

maraf
Copy link
Member

@maraf maraf commented Jul 21, 2023

The goal of this PR is to be able to override both input and output folder of static web assets. We need such configuration for WebAssembly template targeting nodejs/v8/etc. The wwwroot is not correct name in this context.

  • Add new property StaticWebAssetRootPath (default wwwroot) allowing to configure target folder for static web assets
  • Add new item StaticWebAssetSourcePath (default wwwroot) allowing to configure source folder(s) static web assets
  • Usage example in test asset src\Assets\TestProjects\RazorAppWithP2PReferenceWithSwaPaths
  • Add test with complex Razor project
  • Produce error when StaticWebAssetRootPath is not wwwroot for Blazor Wasm project

@maraf maraf added the Area-AspNetCore RazorSDK, BlazorWebAssemblySDK, dotnet-watch label Jul 21, 2023
@maraf maraf self-assigned this Jul 21, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Request triage from a team member label Jul 21, 2023
@ghost
Copy link

ghost commented Jul 21, 2023

Thanks for your PR, @maraf.
To learn about the PR process and branching schedule of this repo, please take a look at the SDK PR Guide.

@maraf maraf marked this pull request as ready for review July 25, 2023 08:31
@maraf maraf requested a review from a team as a code owner July 25, 2023 08:31
@maraf maraf requested a review from radical July 25, 2023 08:31
@maraf maraf removed the untriaged Request triage from a team member label Jul 25, 2023
@maraf maraf added this to the 8.0.1xx milestone Jul 25, 2023
@maraf maraf marked this pull request as draft July 25, 2023 08:53
<ItemGroup>
<FrameworkReference Include="Microsoft.AspNetCore.App" />
</ItemGroup>
</Project>
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 avoid adding an extra test project for this?

@@ -66,6 +66,7 @@ Copyright (c) .NET Foundation. All rights reserved.
<!-- Wire-up static web assets -->
<PropertyGroup>
<ResolveStaticWebAssetsInputsDependsOn>
_EnsureStaticWebAssetRootPath;
Copy link
Member

Choose a reason for hiding this comment

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

I'm fine if we don't block people from changing this. But if we do, this should probably be in ResolveStaticWebAssetsConfiguration

Copy link
Member Author

Choose a reason for hiding this comment

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

I remember you mentioned there are places where this is hardcoded in blazor codebase

<!-- Publish everything under wwwroot, all JSON files, all config files and all Razor files -->
<Content Include="wwwroot\**" ExcludeFromSingleFile="true" CopyToPublishDirectory="PreserveNewest" Exclude="$(DefaultItemExcludes);$(DefaultExcludesInProjectFolder)" />
<!-- Publish everything under @(StaticWebAssetSourcePath), all JSON files, all config files and all Razor files -->
<Content Include="%(StaticWebAssetSourcePath->Identity)\**" ExcludeFromSingleFile="true" CopyToPublishDirectory="PreserveNewest" Exclude="$(DefaultItemExcludes);$(DefaultExcludesInProjectFolder)" />
Copy link
Member

Choose a reason for hiding this comment

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

Why can't this just be $(StaticWebAssetRootPath)\**

Copy link
Member Author

Choose a reason for hiding this comment

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

That was my previous approach. With the current approach (which doesn't work yet :/ ) I'm trying to be able to have a nuget package with public folder, but target app with client folder. So that libraries/packages could be shared (if the actual code make sense to be shared).

Does it make sense to you?

Unfortunatelly this style of msbuild globing doesn't work outside of a target

Comment on lines +14 to +21
<!-- Input and output configuration -->
<PropertyGroup>
<StaticWebAssetRootPath Condition="'$(StaticWebAssetRootPath)' == ''">wwwroot</StaticWebAssetRootPath>
</PropertyGroup>
<ItemGroup Condition="@(StaticWebAssetSourcePath->Count()) == 0">
<StaticWebAssetSourcePath Include="wwwroot" />
</ItemGroup>

Copy link
Member

Choose a reason for hiding this comment

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

Do we actually need to make these input sources configurable? I would rather avoid this, as they are web specific. If people want to follow different conventions, that can be done in the project file directly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Intention described in the comment above..

Comment on lines +43 to +44
for (int i = 0; i < patterns.Length; i++)
matcher.AddInclude(patterns[i]);
Copy link
Member

Choose a reason for hiding this comment

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

brace yourself 😄

@@ -17,7 +17,7 @@ public class DiscoverStaticWebAssets : Task
public ITaskItem[] Candidates { get; set; }

[Required]
public string Pattern { get; set; }
public ITaskItem[] Patterns { get; set; }
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 avoid taking multiple patterns here? This can be called via MSBuild batching from outside instead, and simplifies things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-AspNetCore RazorSDK, BlazorWebAssemblySDK, dotnet-watch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants