Add support for tag helper checksums #9018
Conversation
There was a problem hiding this comment.
I reviewed the bits I understood 👍
Would be interesting to see how hard it would be to use reflection to iterate through tag helper descriptor properties, setting random values, and making sure the checksum changes at each step, to protect against someone forgetting to change this code when new properties are added.
src/Razor/src/Microsoft.AspNetCore.Razor.ProjectEngineHost/CompilationTagHelperResolver.cs
Show resolved
Hide resolved
...hared/Microsoft.AspNetCore.Razor.Utilities.Shared/PooledObjects/PooledDictionaryBuilder`2.cs
Show resolved
Hide resolved
I expect to eventually move the checksum calculations directly onto the tag helper classes. |
...enchmarks/Microsoft.AspNetCore.Razor.Microbenchmarks/Checksums/TagHelperChecksumBenchmark.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.ProjectEngineHost/Utilities/Checksum.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.ProjectEngineHost/Utilities/Checksum.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.ProjectEngineHost/Serialization/JsonDataWriter.cs
Outdated
Show resolved
Hide resolved
| using System.IO; | ||
| #if NETCOREAPP | ||
| using System.Runtime.CompilerServices; | ||
| #endif |
There was a problem hiding this comment.
@davidwengier: Just wanted to note that this is now necessary due to #9014. Not sure if you expected that for multi-targeted projects.
There was a problem hiding this comment.
I hit something similar in that PR, but decided to push on since it was only one case. If it becomes more prevalent or annoying to you, I have no objection to relaxing the rules again.
There was a problem hiding this comment.
Why is this necessary? Is one TFM including a global using the other isn't?
There was a problem hiding this comment.
This using brings in MemoryMarshal for the #if NETCOREAPP code below, so it's unneeded on other TFMs.
| using System; | ||
| #if !NETCOREAPP | ||
| using System.Collections.Generic; | ||
| #endif |
There was a problem hiding this comment.
@davidwengier: Just wanted to note that this is now necessary due to #9014. Not sure if you expected that for multi-targeted projects.
|
@333fred: Let me know if you have further feedback. I believe that I've addressed all of your concerns. |
src/Razor/src/Microsoft.AspNetCore.Razor.ProjectEngineHost/Utilities/Checksum.HashData.cs
Outdated
Show resolved
Hide resolved
ghost
added this to the 
This pull request adds a generalized checksum system based on SHA-256 and derived from a similar system employed in Roslyn. This system isn't used yet, but subsequent PRs will take advantage of checksums to dedupe tag helpers and avoid reconstituting them when unnecessary during serialization.