Skip to content

Implement "Extract to Foo.razor.css" code action #11989

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
Merged
merged 9 commits into from
Jul 3, 2025
Merged

Implement "Extract to Foo.razor.css" code action #11989

merged 9 commits into from
Jul 3, 2025

Conversation

davidwengier
Copy link
Member

Just a little quality-of-life feature I felt I was missing

@davidwengier davidwengier requested a review from a team as a code owner June 29, 2025 00:52
alexgav
alexgav reviewed Jul 1, 2025
View reviewed changes
.../Microsoft.CodeAnalysis.Razor.Workspaces/CodeActions/Razor/ExtractToCssCodeActionResolver.cs
position: (lastLineNumber, lastLineLength),
newText: lastLineNumber == 0
? cssContent
: Environment.NewLine + Environment.NewLine + cssContent)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Environment.NewLine + Environment.NewLine

Another super-nit: could cache two new lines in a static (unless compiler optimizes this)

Copy link
Member Author

@davidwengier davidwengier Jul 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Should make no difference. My understanding is the concat will lower to a string.Concat call, which will pre-allocate a string of the right length, and copy everything in. So there is no intermediate allocation for each piece.

I'm sure Todd or Dustin will correct me if I'm wrong :)

Copy link
Member

Choose a reason for hiding this comment

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

No, the compiler doesn't optimize this, since NewLine can be different at runtime.

It's correct that this'll just be a string.Contact call. it'll allocate a string of size Environment.NewLine.Length + Environment.NewLine.Length + cssContent.Length and perform three copies into the new string to bring the data over. Creating a static with Environment.NewLine + Environment.NewLine to use instead wouldn't be noticeable from a CPU perspective, unless this were some fabulously tight loop executing millions of times. Even then, it probably wouldn't bubble up.

alexgav
alexgav reviewed Jul 1, 2025
View reviewed changes
.../Microsoft.CodeAnalysis.Razor.Workspaces/CodeActions/Razor/ExtractToCssCodeActionResolver.cs
GetLastLineNumberAndLength(stream, bufferSize: 4096, out lastLineNumber, out lastLineLength);
}

private static void GetLastLineNumberAndLength(Stream stream, int bufferSize, out int lastLineNumber, out int lastLineLength)
Copy link
Contributor

Choose a reason for hiding this comment

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

GetLastLineNumberAndLength

nit: Seems like this could go to StreamHelpers.cs or StreamExtensions.cs somewhere, and then you wouldn't need TestAccessor

Copy link
Member Author

Choose a reason for hiding this comment

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

It could, but also feels pretty niche to me so I'm not sure

alexgav
alexgav approved these changes Jul 1, 2025
View reviewed changes
Copy link
Contributor

@alexgav alexgav left a comment

Choose a reason for hiding this comment

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

:shipit:

DustinCampbell
DustinCampbell approved these changes Jul 2, 2025
View reviewed changes
...c/Microsoft.CodeAnalysis.Razor.Workspaces/CodeActions/Models/ExtractToCssCodeActionParams.cs
Comment on lines +10 to +20
[JsonPropertyName("extractStart")]
public int ExtractStart { get; set; }

[JsonPropertyName("extractEnd")]
public int ExtractEnd { get; set; }

[JsonPropertyName("removeStart")]
public int RemoveStart { get; set; }

[JsonPropertyName("removeEnd")]
public int RemoveEnd { 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.

💭 Broader question: Should we just be using TextSpan for "start/end" pairs of properties like these? After all, we generally assign them with TextSpan.Start and TextSpan.End. If you agree, that'd probably be better in a separate change since here are several "params" classes with a similar shape.

Copy link
Member Author

Choose a reason for hiding this comment

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

These need to be JSON serializable so we'd have to use Range, which means it's probably a wash to me, but I don't mind it. One day I'll beat these provider and resolver classes into a shape I like, and have things spread across less files, so maybe then? :)

Copy link
Member

Choose a reason for hiding this comment

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

Just wanted to ask the question. I'd forgotten that [DataMember] isn't supported by System.Text.Json, but I'm glad you're keeping track of those details. 😄

It does seem like it'd be pretty easy to add RazorTextSpan that is JSON-serializable and implicitly convertible to/from TextSpan, if you fancy that idea.

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 forgot about that type!

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I didn't realize we already had. FWIW, it looks like that type should probably be a record struct rather than a sealed record to avoid heap allocations. Then, it's just a matter of making it implicitly convertible to/from TextSpan it should be super easy to use.

.../Microsoft.CodeAnalysis.Razor.Workspaces/CodeActions/Razor/ExtractToCssCodeActionResolver.cs
position: (lastLineNumber, lastLineLength),
newText: lastLineNumber == 0
? cssContent
: Environment.NewLine + Environment.NewLine + cssContent)]
Copy link
Member

Choose a reason for hiding this comment

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

No, the compiler doesn't optimize this, since NewLine can be different at runtime.

It's correct that this'll just be a string.Contact call. it'll allocate a string of size Environment.NewLine.Length + Environment.NewLine.Length + cssContent.Length and perform three copies into the new string to bring the data over. Creating a static with Environment.NewLine + Environment.NewLine to use instead wouldn't be noticeable from a CPU perspective, unless this were some fabulously tight loop executing millions of times. Even then, it probably wouldn't bubble up.

@davidwengier
PR Feedback
6199e76
@davidwengier
Merge remote-tracking branch 'upstream/main' into ExtractToCss
4dd2d93 
# Conflicts:
#	src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Protocol/LanguageServerConstants.cs
#	src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Resources/SR.resx
@davidwengier
Remove invalid test. We filter non-components our in the provider
292b41a
@davidwengier davidwengier merged commit f7f9a7c into dotnet:main Jul 3, 2025
11 checks passed
@davidwengier davidwengier deleted the ExtractToCss branch July 3, 2025 00:23
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Jul 3, 2025
@dotnet-bot dotnet-bot mentioned this pull request Jul 4, 2025
Closed
@davidwengier davidwengier mentioned this pull request Jul 14, 2025
Merged
@RikkiGibson RikkiGibson modified the milestones: Next, 18.0 P1 Aug 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DustinCampbell DustinCampbell DustinCampbell approved these changes

@alexgav alexgav alexgav approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
18.0 P1
Development

Successfully merging this pull request may close these issues.
4 participants
@davidwengier @DustinCampbell @alexgav @RikkiGibson