-
Notifications
You must be signed in to change notification settings - Fork 739
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.
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
Adding BufferedHtmlContent and removing BufferEntryCollection. #32
Conversation
/// <summary> | ||
/// Enumerable object collection which knows how to write itself. | ||
/// </summary> | ||
internal class BufferedHtmlContent : IHtmlContent, IEnumerable<object> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the IEnumerable<object>
for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because of the type of Entries
? It can be both string
and IHtmlContent
..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be IEnumerable<string>
to avoid the recursion in callers. That is, copy over BufferEntryEnumerator
from the deleted class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note BufferEntryEnumerator
handled nested BufferEntryCollection
s just fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this IEnumerable<object>
exposure used anywhere other than the DefaultTagHelperContent
"check" methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it seems like it doesn't need to be an IEnumerable<anything>
, does it? It merely needs to have an IEnumerable<something>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main counter-argument to not implementing IEnumerable<T>
would have been the strangeness of a BufferEntryCollection
that did not expose itself as a collection. Given the rename 👍
The Entries
property is already public
(called internal
but this is an internal
class so that's irrelevant). That means the IEnumerable<object>
implementation is redundant. Just get rid of the internal for testing
comment if product code e.g. the DefaultTagHelperOutput
implementation uses Entries
.
One option to avoid any product code using the enumeration would be DefaultTagHelperOutput
calculating the IsWhiteSpace
and IsEmpty
values in the Append()
, AppendFormat()
and Clear()
methods. Bit more code and that code wouldn't really be needed in the 80% case. But the code would be rather straighforward. The worst case:
public override TagHelperContent Append(IHtmlContent htmlContent)
{
var tagHelperContent = htmlContent as TagHelperContent;
if (tagHelperContent != null)
{
if (!tagHelperContent.IsEmpty)
{
_isEmpty = false;
}
if (!tagHelperContent.IsWhiteSpace)
{
_isWhiteSpace = false;
}
}
else
{
var value = htmlContent as string;
if (value == null)
{
// Mentioned GetRawString() in Razor PR.
value = htmlContent.GetRawString();
}
if (string.IsNullOrEmpty(value))
{
// no-op
}
else if (string.IsNullOrWhiteSpace(value))
{
_isEmpty = false;
}
else
{
_isEmpty = false;
_isWhiteSpace = false;
}
}
Buffer.Append(htmlContent);
return this;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value of that change for whitespace computation seems rather dubious...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above Append
method is assuming that TagHelperContent.Append
can be called with string
and TagHelperContent
only. If we change TagBuilder
to implement IHtmlContent
as pointed out here, that would be Append
ed to TagHelperContent
as well. We would end up converting TagBuilder
s (and other IHtmlContent
s) to strings even if IsEmpty
/IsWhitespace
is never called. So I think this is a bad idea.
The enumeration is not used anywhere apart from the check methods in DefaultTagHelperContent
. I'll remove BufferedHtmlContent
from being an IEnumerable
. Instead I'll add constructor like:
public BufferedHtmlContent(List<object> store)
This would initialize the BufferedHtmlContent
's backing store with the one passed in. With this, I can solve the enumeration problem and also conversion to strings happen only when necessary. What do you think?
⌚ |
/// Appends a <see cref="IHtmlContent"/> to the collection. | ||
/// </summary> | ||
/// <param name="htmlContent">The <see cref="IHtmlContent"/> to be appended.</param> | ||
public BufferedHtmlContent Append(IHtmlContent htmlContent) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How much does performance improve if you special case appending another BufferedHtmlContent
instance as BufferEntryCollection
did before -- appending the passed instance's entries rather than sticking the BufferedHtmlCotnent
itself into the collection? That would avoid some recursion in WriteTo()
and allow the old instance to be garbage collected earlier.
- Making BufferedHtmlContent non-enumerable. - Updating tests and comments.
Updated.. |
One high-level question: MVC has previously been quite conservative about HTML encoding. It generally HTML encodes everything except for a few well-known cases e.g. This new system does the opposite: No |
@dougbu what exactly do you mean? What was previously encoded that would no longer be encoded? |
I didn't say the system wasn't working as @sornaks has rewritten it. I said the approach has switched around -- which seems less safe and may confuse users. This PR takes us from "encode this unless it's an It's possible to create a system that behaves more like MVC 5 and earlier MVC 6 milestones. In that world |
Sorry, still not sure I understand This stuff is basically all plumbing. How, whether on purpose or by accident, would something that would previously have been encoded, no longer be encoded? What type of error, whether by us, or by a customer, would cause an issue? |
E.g. user appends a |
Wait a sec - what? That sounds completely broken to me. I thought the whole point of Let's discuss Monday with the right set of folks. |
Checked in. Thanks! 051a5f7 |
No description provided.