-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Write import map to html files #46233
base: main
Are you sure you want to change the base?
Conversation
src/StaticWebAssetsSdk/Targets/Microsoft.NET.Sdk.StaticWebAssets.HtmlImportMap.targets
Outdated
Show resolved
Hide resolved
I would settle on one syntax. Rather than using the HTML comment, I would detect <script type="importmap"></script> or |
src/StaticWebAssetsSdk/Targets/Microsoft.NET.Sdk.StaticWebAssets.HtmlImportMap.targets
Outdated
Show resolved
Hide resolved
src/StaticWebAssetsSdk/Targets/Microsoft.NET.Sdk.StaticWebAssets.HtmlImportMap.targets
Outdated
Show resolved
Hide resolved
<PropertyGroup> | ||
<ResolveBuildRelatedStaticWebAssetsDependsOn> | ||
$(ResolveStaticWebAssetsInputsDependsOn); | ||
_UpdateHtmlFiles; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: This needs to happen also before the service-worker-manifest gets generated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is that? The html documents are not part of service worker assets manifest, aren't they?
src/StaticWebAssetsSdk/Targets/Microsoft.NET.Sdk.StaticWebAssets.HtmlImportMap.targets
Outdated
Show resolved
Hide resolved
src/StaticWebAssetsSdk/Tasks/GenerateStaticWebAssetEndpointsManifest.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall the direction looks good. The main pieces of feedback are:
- Settle on a single syntax. I would avoid using
@Assets
as it might set the wrong expectations.- Maybe just adding
#[.{fingerprint}]
which is something we can interpret and recognize.
- Maybe just adding
- I would avoid blanket fingerprinting all the JS assets, and only limit things to stuff we know about we'll get right.
- I would avoid all the copy/paste from the runtime and work directly with the list of endpoints to generate the importmap.
The main missing piece here is also the publish bit, which needs to be accounted for. It'll be good if we follow the same template as https://github.com/dotnet/sdk/blob/main/src/StaticWebAssetsSdk/Targets/Microsoft.NET.Sdk.StaticWebAssets.JSModules.targets where there is
- Resolve(Feature)(Build|Publish)Configuration:
- Figures out what assets/files we need to consider
- Generate(Feature)(Build|Publish)StaticWebAssets:
- Does the work of generating any outputs.
- Resolve(Feature(Build|Publish)StaticWebAssets.
- Defines the assets and endpoints for the outputs.
I was thinking in something like `="(.#[.].*)" would be enough, wouldn't it? Then look at $1. |
I had the same idea later, originally thought it might be limiting to just html attribute, but maybe that's actually good. I'll update the PR to that pattern |
Publish test will fail until the runtime PR is resolved |
src/BlazorWasmSdk/Targets/Microsoft.NET.Sdk.BlazorWebAssembly.6_0.targets
Outdated
Show resolved
Hide resolved
|
||
<PropertyGroup> | ||
|
||
<!-- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is explaining the execution order ? Maybe you can clarify that
<MakeDir Directories="$(_BuildImportMapHtmlPath)"/> | ||
|
||
<ItemGroup> | ||
<_HtmlStaticWebAssets Include="@(StaticWebAsset)" Condition="'%(AssetKind)' != 'Publish' and '%(Extension)' == '.html'" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also do older .htm
?
OutputPath="$(_BuildImportMapHtmlPath)"> | ||
<Output TaskParameter="HtmlCandidates" ItemName="_HtmlCandidates" /> | ||
<Output TaskParameter="HtmlFilesToRemove" ItemName="_HtmlFilesToRemove" /> | ||
<Output TaskParameter="FileWrites" ItemName="FileWrites" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this friendly to incremental build ?
if (asset.Label != null) | ||
{ | ||
imports ??= []; | ||
imports[$"./{asset.Label}"] = $"./{asset.Url}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can asset label contain some interesting characters ? Does the URL need URL encoding ?
@@ -44,4 +45,53 @@ public void Build_FingerprintsContent_WhenEnabled() | |||
AssertManifest(manifest1, expectedManifest); | |||
AssertBuildAssets(manifest1, outputPath, intermediateOutputPath); | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a test that covers Blazor too? (Hosted and Standalone) or is that covered elsewhere?
<ResolveBuildRelatedStaticWebAssetsDependsOn> | ||
$(ResolveBuildRelatedStaticWebAssetsDependsOn); | ||
ResolveHtmlImportMapBuildStaticWebAssets; | ||
</ResolveBuildRelatedStaticWebAssetsDependsOn> | ||
<ResolveCompressedFilesDependsOn> | ||
$(ResolveCompressedFilesDependsOn); | ||
ResolveHtmlImportMapBuildStaticWebAssets | ||
</ResolveCompressedFilesDependsOn> | ||
<ResolveHtmlImportMapBuildStaticWebAssetsDependsOn> | ||
GenerateHtmlImportMapBuildStaticWebAssets; | ||
$(ResolveHtmlImportMapBuildStaticWebAssetsDependsOn) | ||
</ResolveHtmlImportMapBuildStaticWebAssetsDependsOn> | ||
<GenerateHtmlImportMapBuildStaticWebAssetsDependsOn> | ||
ResolveHtmlImportMapBuildConfiguration; | ||
$(GenerateHtmlImportMapBuildStaticWebAssetsDependsOn) | ||
</GenerateHtmlImportMapBuildStaticWebAssetsDependsOn> | ||
|
||
<!-- | ||
ResolvePublishRelatedStaticWebAssets | ||
ResolveHtmlImportMapPublishStaticWebAssets | ||
GenerateHtmlImportMapPublishStaticWebAssets | ||
ResolveHtmlImportMapPublishConfiguration | ||
--> | ||
<ResolvePublishRelatedStaticWebAssetsDependsOn> | ||
$(ResolvePublishRelatedStaticWebAssetsDependsOn); | ||
ResolveHtmlImportMapPublishStaticWebAssets | ||
</ResolvePublishRelatedStaticWebAssetsDependsOn> | ||
<ResolveHtmlImportMapPublishStaticWebAssetsDependsOn> | ||
GenerateHtmlImportMapPublishStaticWebAssets; | ||
$(ResolveHtmlImportMapPublishStaticWebAssetsDependsOn) | ||
</ResolveHtmlImportMapPublishStaticWebAssetsDependsOn> | ||
<GenerateHtmlImportMapPublishStaticWebAssetsDependsOn> | ||
ResolveHtmlImportMapPublishConfiguration; | ||
$(GenerateHtmlImportMapPublishStaticWebAssetsDependsOn) | ||
</GenerateHtmlImportMapPublishStaticWebAssetsDependsOn> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to ensure that this runs before the service worker is generated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also needs to run before compression happens
<ItemGroup> | ||
<StaticWebAsset Remove="@(_HtmlFilesToRemove)" /> | ||
<StaticWebAsset Include="@(_UpdatedHtmlStaticWebAssets)" /> | ||
<StaticWebAssetEndpoint Include="@(_UpdatedHtmlStaticWebAssetsEndpoint)" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we remove the old endpoints too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but I think there are a few additional things we should solve before getting it merged.
|
||
<Target Name="ResolveHtmlImportMapBuildConfiguration"> | ||
<PropertyGroup> | ||
<_BuildImportMapHtmlPath>$([MSBuild]::NormalizeDirectory($(IntermediateOutputPath), 'importmaphtml', 'build'))</_BuildImportMapHtmlPath> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
if (content != outputContent) | ||
{ | ||
string outputPath = Path.Combine(OutputPath, Path.GetRandomFileName() + item.GetMetadata("Extension")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't do this. Use a hash of the content if anything, but don't use Random
as this makes things non-deterministic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rewrites html files to include import map and put fingerprint into referenced assets
<script type="importmap"></script>
"main#[{.fingerprint}].js"
. Unfortunately root scripts event withtype=module
are not affected by import map and inline script with import has other disadvantages. So we need a syntax for the rewrite. This syntax is the same as can be used in server process razor files.Enabled by
WriteImportMapToHtml=true
. At the moment the feature is opt-in, but I believe we should make it opt-out, possibly later in the cycle if we gather any feedback.For Blazor scenarios it solves two issues
Open questions
blazorwasm
templateSample input & output for
wasmbrowser
based index.htmlInput
Output