Skip to content

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Aug 6, 2025

The ScriptTagHelper was incorrectly suppressing <script type="importmap"> tags that contained explicit user-provided content, causing them to render as nothing in ASP.NET Core 9.0.

Problem

When users wrote importmap scripts with explicit content like:

<script type="importmap">
{
  "imports": {
    "jquery": "https://code.jquery.com/jquery.js"
  }
}
</script>

The ScriptTagHelper would completely suppress the output instead of preserving the user's content. This regression broke existing applications that relied on explicit importmap definitions.

Root Cause

The tag helper was being overly aggressive - it processed all script tags with type="importmap" and when no ImportMapDefinition was found in endpoint metadata, it would call output.SuppressOutput() regardless of whether the user had provided explicit content.

Solution

Modified the ScriptTagHelper to check for existing content first using await output.GetChildContentAsync():

  • If existing content is found (not empty/whitespace) → preserve it and don't process further
  • If no existing content → apply normal importmap logic (generate from definition or suppress if none found)

This approach ensures that user-provided content is never replaced, while maintaining all existing functionality for automatic importmap generation.

Implementation Details

  • Added ProcessAsync method that properly handles the async content checking
  • Maintained backward compatibility by keeping the original Process method
  • Updated public API to reflect the new async method

Testing

Added comprehensive tests to verify:

  • Explicit importmap content is preserved when users provide it
  • Output is still suppressed when no content and no definition exists
  • All existing functionality remains intact

Fixes #58973.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

@Copilot Copilot AI changed the title [WIP] Razor regression in ASP .NET Core 9.0 (script type=importmap and others) Fix ScriptTagHelper regression with importmap scripts containing explicit content Aug 6, 2025
@Copilot Copilot AI requested a review from MackinnonBuck August 6, 2025 17:45
Copilot finished work on behalf of MackinnonBuck August 6, 2025 17:45
Comment on lines 250 to 266
// No importmap found. Only suppress output if this was intended to be
// an automatically generated importmap (i.e., when asp-importmap was used).
// If the user provided explicit content without asp-importmap, let it render as-is.
if (ImportMap != null || context.AllAttributes.ContainsName(ImportMapAttributeName))
{
output.SuppressOutput();
return;
}
// Let the tag render as-is by continuing with normal processing
// Don't return here, let normal attribute copying happen
}
else
{
output.TagName = "script";
output.TagMode = TagMode.StartTagAndEndTag;
output.Attributes.SetAttribute("type", "importmap");
output.Content.SetHtmlContent(importMap.ToString());
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is the right fix.

If the goal is to do nothing if there is already content on the script, then what should happen is:

  • Call var originalContent = await output.GetChildContentAsync().
  • Check for content.IsEmptyOrWhitespace
  • Set the import map only in that situation.

Copy link
Member

Choose a reason for hiding this comment

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

Note that we still want to process all <script type="importmap"></script> but the fix is only to avoid replacing existing content if there's already data the developer put it.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in commit 15776b3. I've implemented the approach you suggested:

  1. Call await output.GetChildContentAsync() to get existing content
  2. Check if content is empty or whitespace using IsEmptyOrWhiteSpace
  3. If content exists, preserve it and return early
  4. If no content exists, apply the normal importmap logic

The fix now properly processes all <script type="importmap"></script> tags but avoids replacing existing content when developers have provided their own importmap data. I've maintained backward compatibility by keeping the original Process method that calls the new ProcessAsync method.

@Copilot Copilot AI changed the title Fix ScriptTagHelper regression with importmap scripts containing explicit content Fix ScriptTagHelper regression by checking for existing content before processing importmaps Aug 11, 2025
@Copilot Copilot AI requested a review from javiercn August 11, 2025 13:57
Copilot finished work on behalf of javiercn August 11, 2025 13:57
@MackinnonBuck MackinnonBuck marked this pull request as ready for review September 5, 2025 18:52
@Copilot Copilot AI review requested due to automatic review settings September 5, 2025 18:52
@MackinnonBuck MackinnonBuck requested a review from a team as a code owner September 5, 2025 18:52
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a regression in the ScriptTagHelper where it was incorrectly suppressing <script type="importmap"> tags that contained explicit user-provided content. The fix ensures user-provided importmap content is preserved while maintaining automatic importmap generation functionality.

  • Modified ScriptTagHelper to check for existing content before processing importmaps
  • Added async ProcessAsync method to properly handle content checking
  • Updated test infrastructure to support child content testing

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
src/Mvc/Mvc.TagHelpers/src/ScriptTagHelper.cs Changed Process to ProcessAsync and added logic to preserve existing content in importmap scripts
src/Mvc/Mvc.TagHelpers/test/ScriptTagHelperTest.cs Added comprehensive tests for content preservation and updated helper methods to support child content
src/Mvc/Mvc.TagHelpers/src/PublicAPI.Unshipped.txt Added new public API entry for the ProcessAsync method override

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.

Looks good to me!

@MackinnonBuck
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@MackinnonBuck
Copy link
Member

Also manually confirmed the fix.

@MackinnonBuck MackinnonBuck merged commit 641f0b4 into main Sep 10, 2025
30 checks passed
@MackinnonBuck MackinnonBuck deleted the copilot/fix-58973 branch September 10, 2025 20:21
@dotnet-policy-service dotnet-policy-service bot added this to the 11.0-preview1 milestone Sep 10, 2025
@MackinnonBuck
Copy link
Member

/backport to release/10.0

Copy link
Contributor

Started backporting to release/10.0: https://github.com/dotnet/aspnetcore/actions/runs/17628140064

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Razor regression in ASP .NET Core 9.0 (script type=importmap and others)

3 participants