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.

Sign up for GitHub

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

Jump to bottom

Move asset compression to the StaticWebAssets SDK #31559

Merged
merged 17 commits into from
Apr 14, 2023
Merged

Move asset compression to the StaticWebAssets SDK #31559

merged 17 commits into from
Apr 14, 2023

Conversation

MackinnonBuck
Copy link
Member

@MackinnonBuck MackinnonBuck commented Mar 30, 2023 •
edited
Loading

Move asset compression to the StaticWebAssets SDK

This PR moves the asset compression feature from the BlazorWebAssembly SDK to the StaticWebAssets SDK.

List of major changes

  1. Separated the compression step from the "resolution" step
    • Before this PR, the resolution of assets to compress and the compression of said assets would happen in the same MSBuild target. This PR separates these steps by allowing the specification of a "compression configuration" that the StaticWebAssets SDK picks up to resolve the final compressed StaticWebAsset definitions before the actual compression happens.
      • The compression configuration specifies:
        • The compression format
        • The stage to perform the compression
        • Optional include/exclude patterns to match files to be compressed
      • Assets can also be explicitly added to a configuration by adding items to the AssetsToCompress item group
      • Compression occurs after all StaticWebAsset items have been added
  2. Added unit tests for the new ResolveCompressedAssets task
  3. Copied the existing BrotliCompress and GZipCompress tasks to M.NET.Sdk.StaticWebAssets.Tasks and updated them accordingly
    • This was a copy instead of a move in order to retain backwards compatibility
  4. Removed the now unused ComputeBlazorFilesToCompress task and its corresponding tests.

Fixes dotnet/aspnetcore#46998

@MackinnonBuck MackinnonBuck requested a review from javiercn March 30, 2023 23:56
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-AspNetCore RazorSDK, BlazorWebAssemblySDK, dotnet-watch untriaged Request triage from a team member labels Mar 30, 2023
@ghost
Copy link

ghost commented Mar 30, 2023

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

@MackinnonBuck
Copy link
Member Author

Source build failures seem unrelated. They're happening in main, too.

@MackinnonBuck MackinnonBuck marked this pull request as ready for review April 5, 2023 22:30
javiercn
javiercn reviewed Apr 13, 2023
View reviewed changes
src/StaticWebAssetsSdk/Targets/Microsoft.NET.Sdk.StaticWebAssets.ScopedCss.5_0.targets
@@ -190,7 +190,7 @@ Integration with static web assets:
<MakeDir Directories="$(_ScopedCssIntermediatePath)" />
<RewriteCss
FilesToTransform="@(_ScopedCss)"
ToolAssembly="$(_StaticWebAssetsSdkToolAssembly)"
ToolAssembly="$(_StaticWebAssetsSdkRazorToolAssembly)"
Copy link
Member

Choose a reason for hiding this comment

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

Is Razor needed for a reason here?

javiercn
javiercn approved these changes Apr 13, 2023
View reviewed changes
Copy link
Member

@javiercn javiercn left a comment

Choose a reason for hiding this comment

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

A few minor details, but otherwise looks great!

@maraf maraf mentioned this pull request Apr 13, 2023
Merged
@MackinnonBuck MackinnonBuck merged commit cf2977b into main Apr 14, 2023
@MackinnonBuck MackinnonBuck deleted the mbuck/static-web-assets-compression branch April 14, 2023 18:02
@maraf maraf mentioned this pull request Apr 17, 2023
Merged
@radical radical mentioned this pull request Apr 18, 2023
Merged
radical added a commit that referenced this pull request Apr 19, 2023
@radical
[wasm] Fix regression in blazor+aot
9d32226 
- For blazor AOT, the blazor targets explicitly skip
  `Microsoft.JSInterop.WebAssembly.dll` in `_GatherBlazorFilesToPublish` target.
  - it is done by marking the assembly item in
    `@(WasmAssembliesToBundle)` with `%(AOT_InternalForceToInterpret)="true"`.

- this target needs to run after `_GatherWasmFilesToPublish` which builds
  the list of assemblies to AOT in `@(WasmAssembliesToBundle)`

- the order of these two targets was corrected in #31640
- but the changes in #31559 added the
  target twice, resulting in the order:

  `_GatherBlazorFilesToPublish;_GatherWasmFilesToPublish;_GatherBlazorFilesToPublish`

  1. in which the first `_GatherBlazorFilesToPublish` tries to remove
     the assembly from an empty list
  2. then `_GatherWasmFilesToPublish` populates the list
  3. and the last instance of `_GatherBlazorFilesToPublish` target
     doesn't run because it has already run before in (2).
- thus the assembly never gets removed from the list.

Fixes: dotnet/runtime#85010
radical added a commit that referenced this pull request Apr 19, 2023
@radical
[wasm] Fix regression in blazor+aot (#31870)
79b198d 
- For blazor AOT, the blazor targets explicitly skip `Microsoft.JSInterop.WebAssembly.dll` in `_GatherBlazorFilesToPublish` target.
  - it is done by marking the assembly item in `@(WasmAssembliesToBundle)` with `%(AOT_InternalForceToInterpret)="true"`.

- this target needs to run after `_GatherWasmFilesToPublish` which builds the list of assemblies to AOT in `@(WasmAssembliesToBundle)`

- the order of these two targets was corrected in #31640
- but the changes in #31559 added the target twice, resulting in the order:

  `_GatherBlazorFilesToPublish;_GatherWasmFilesToPublish;_GatherBlazorFilesToPublish`

  1. in which the first `_GatherBlazorFilesToPublish` tries to remove the assembly from an empty list
  2. then `_GatherWasmFilesToPublish` populates the list
  3. and the last instance of `_GatherBlazorFilesToPublish` target doesn't run because it has already run before in (2).
- thus the assembly never gets removed from the list.

Fixes: dotnet/runtime#85010
@MackinnonBuck MackinnonBuck mentioned this pull request Apr 19, 2023
Closed
@sebastienros sebastienros mentioned this pull request Apr 24, 2023
Closed
@MackinnonBuck MackinnonBuck mentioned this pull request Apr 25, 2023
Merged
3 tasks
MackinnonBuck added a commit that referenced this pull request May 8, 2023
@MackinnonBuck
Follow-up to #31559 (#32047)
9314a55
This was referenced Sep 1, 2023
Closed
Merged
@javiercn javiercn mentioned this pull request Oct 4, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maraf maraf maraf approved these changes

@javiercn javiercn javiercn approved these changes

@lewing lewing Awaiting requested review from lewing lewing is a code owner

@radical radical Awaiting requested review from radical

@pavelsavara pavelsavara Awaiting requested review from pavelsavara pavelsavara is a code owner

Assignees
No one assigned
Labels
Area-AspNetCore RazorSDK, BlazorWebAssemblySDK, dotnet-watch untriaged Request triage from a team member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Move asset compression to the Static Web Assets SDK from the Blazor Webassembly SDK
3 participants
@MackinnonBuck @javiercn @maraf