Add support for static files to dotnet-watch #15405
Conversation
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
pranavkm
force-pushed
the
prkrishn/static-files
branch
2 times, most recently
from
dd739d8 to
bdf3347
Compare
| } | ||
|
|
||
| function updateAllLocalCss() { | ||
| [...document.querySelectorAll('link')] |
There was a problem hiding this comment.
Super nit: We might want to filter by rel='stylesheet' to avoid selecting links to non-stylesheet assets.
Probably an edge case so feel free to ignore.
|
|
||
| const newElement = styleElement.cloneNode(); | ||
| const href = styleElement.href; | ||
| newElement.href = href.split('?', 1)[0] + `?nonce=${Date.now()}`; |
There was a problem hiding this comment.
Is there a chance href.split('?', 1) can be empty?
| namespace Microsoft.DotNet.Watcher.Tools | ||
| { | ||
| internal static class AssertEx | ||
| public static class AssertEx |
There was a problem hiding this comment.
Why are we changing the visibility here?
| PackagePath="tools\$(SdkTargetFramework)\any\middleware" /> | ||
| </ItemGroup> | ||
| </Target> | ||
| <ProjectReference |
| public ProcessSpec ProcessSpec { get; set; } | ||
|
|
||
| public IFileSet FileSet { get; set; } | ||
| public FileSet FileSet { get; set; } |
There was a problem hiding this comment.
Why do we want to use the concrete type instead of the interface here?
There was a problem hiding this comment.
Nevermind. I see that the interface was removed.
There was a problem hiding this comment.
It's a POCO type, there's really no need for there to be an interface and a concrete implementation. Kinda gets in the way since you have to update two places every time you make a change to the contract here.
jkotalik
left a comment
jkotalik
left a comment
There was a problem hiding this comment.
The FileSetSerializer is the only new par of this PR correct?
LGTM otherwise.
| { | ||
| public ITaskItem[] WatchFiles { get; set; } | ||
|
|
||
| public bool IsNetCoreApp31OrNewer { get; set; } |
There was a problem hiding this comment.
Reminder where this is used again?
There was a problem hiding this comment.
We use it to determine if we can inject the browser refresh middleware. It compiles against netcoreapp3.1 so we have to make sure it's at least that
pranavkm
force-pushed
the
prkrishn/static-files
branch
from
8fffe5f to
89d9a4c
Compare
pranavkm
added
auto-merge
|
Hello @pranavkm! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
|
Apologies, while this PR appears ready to be merged, I've been configured to only merge when all checks have explicitly passed. The following integrations have not reported any progress on their checks and are blocking auto-merge:
These integrations are possibly never going to report a check, and unblocking auto-merge likely requires a human being to update my configuration to exempt these integrations from requiring a passing check. Give feedback on thisFrom the bot dev teamWe've tried to tune the bot such that it posts a comment like this only when auto-merge is blocked for exceptional, non-intuitive reasons. When the bot's auto-merge capability is properly configured, auto-merge should operate as you would intuitively expect and you should not see any spurious comments. Please reach out to us at [email protected] to provide feedback if you believe you're seeing this comment appear spuriously. Please note that we usually are unable to update your bot configuration on your team's behalf, but we're happy to help you identify your bot admin. |
This is largely a port of dotnet/aspnetcore#27744 with one large difference - I added an MSBuild task to serialize the list of files from MSBuild -> dotnet-watch. In the PR this was done by relying on CLI parsing.