Added view component tag helper updates. #823
Conversation
|
Hi @cjqian, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! The agreement was validated by .NET Foundation and real humans are currently evaluating your PR. TTYL, DNFBOT; |
|
No tests? |
| } | ||
| } | ||
|
|
||
| return new CodeGeneratorResult(writer.GenerateCode(), writer.LineMappingManager.Mappings); | ||
| } | ||
|
|
||
| protected virtual void BuildAfterExecuteContent(CSharpCodeWriter writer, IList<Chunk> chunks) | ||
| { |
There was a problem hiding this comment.
What happens if you mutate the chunks list here (add/remove)?
There was a problem hiding this comment.
Do you mean that we would DecorateChunks at BuildAfterExecuteContent?
For view component tag helpers, we need to mutate their chunks such that the type name includes the name of the page (ex. AspNetCore._Views_Index_Cshtml) since we'll be appending the after execute content within that class.
This needs to be done before the logic in lines 68-71, since that's when the type names of the chunks are written in the generated code. We could actually call DecorateChunks directly before that instead of at line 51?
There was a problem hiding this comment.
Both here and in DecorateChunks, what happens if I add a new chunk to the list of chunks? I know the view component tag helpers story required mutating the existing chunks, but what happens if new ones are added or removed. Should this list be readonly or are there valid things you could do.
Similarly, what happens if chunks are removed. Will code that runs later blow up?
There was a problem hiding this comment.
Oh, understood. I don't think anything would blow up... It would be similar to adding/deleting parts of the .cshtml file, maybe. Worst case, you could change the content of the chunks and rewrite the view? If the chunks were invalid, perhaps things would just not render? (I'm not sure on this, what do you think?) The chunk list has to be modifiable in our current implementation so we can modify the type names. Is this valid?
There was a problem hiding this comment.
Well, DecorateChunks() is about mucking w/ the chunks collection and this method is about writing stuff out. Suggest this method use IReadOnlyList<Chunk> chunks to make things more clear.
|
@davidfowl Tests for which part? We're just exposing |
|
@cjqian Tests verifying that the new public method you exposed does what you expect. Also that the new virtual method gets called with the right arguments. |
| @@ -711,7 +711,7 @@ private static bool IsAccessibleProperty(PropertyInfo property) | |||
| /// ONE1TWO2THREE3 => one1two2three3 | |||
| /// First_Second_ThirdHi => first_second_third-hi | |||
| /// </example> | |||
There was a problem hiding this comment.
Need to fix the <example> since this is now public. Right now all the lines will be shown as one messy line. Easiest to surround the lines w/ <code>...</code>.
|
Like the coverage of the new tests and the new extension points. My comments suggest alternatives but I don't such anything that's Just Wrong™️. |
|
⌚ |
| /// Provides an entry point to append code (after execute content) to a generator razor class. | ||
| /// </summary> | ||
| /// <param name="writer"></param> | ||
| /// <param name="chunks"></param> |
|
⌚ almost done! |
| } | ||
| } | ||
|
|
||
| return new CodeGeneratorResult(writer.GenerateCode(), writer.LineMappingManager.Mappings); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Provides an entry point to append code (after execute content) to a generator razor class. |
|
⌚ for one last touch-up of the doc comments. |
|
🆙 📅 |
| /// <summary> | ||
| /// Provides an entry point to modify chunk data before code generation. | ||
| /// </summary> | ||
| /// <param name="context">The <see cref="CodeGeneratorContext"/> of the chunks."/> </param> |
There was a problem hiding this comment.
Last "/> is a typo. Remove those 3 characters.
| /// </summary> | ||
| /// <param name="writer">The <see cref="CSharpCodeWriter"/> to receive the additional content.</param> | ||
| /// <param name="chunks">The list of <see cref="Chunk"/>s for the generated program.</param> | ||
| protected virtual void BuildAfterExecuteContent(CSharpCodeWriter writer, IList<Chunk> chunks) |
There was a problem hiding this comment.
Forgot: Does IReadOnlyList<Chunk> not work for your feature?
There was a problem hiding this comment.
Decided offline to leave this as is b/c Tree.Children is an IList<Chunk> and creating another List<Chunk> isn't worth it.
|
|
| /// </summary> | ||
| /// <param name="context">The <see cref="CodeGeneratorContext"/> of the chunks."/> </param> | ||
| /// <param name="chunks">The list of <see cref="Chunk"/>s for the generated program.</param> | ||
| protected virtual void DecorateChunks(CodeGeneratorContext context, IList<Chunk> chunks) |
There was a problem hiding this comment.
So we don't technically need this piece. MVC already semi-decorates the chunk tree by merging in _ViewImports chunks.
|
🆙 📅 |
| .Write(typeName) | ||
| .Write(" ") | ||
| .Write(name) | ||
| .Write(" {") |
There was a problem hiding this comment.
.WriteLine(" { get; set; }"); 👍
|
🆙 📅 |
|
|
1 similar comment
|
|
cjqian
force-pushed
the
ViewComponentTagHelperUpdates
branch
from
cc9c536 to
2deafd4
Compare
ToHtmlCasefor use in creating tag names for view component tag helper descriptors.WriteAutoPropertyDeclarationmethod.DecorateChunksas an opportunity to modify chunks before parsing in code generation, andBuildAfterExecuteContentto append extra generated code. Check these out in action here! (Still in progress, but hopefully you get the idea.)