Skip to content
This repository has been archived by the owner on Dec 14, 2018. It is now read-only.

Reacting to Razor changes - TagHelperOutput is writing to TextWriter instead of string. #2099

Closed
wants to merge 4 commits into from

Conversation

sornaks
Copy link

@sornaks sornaks commented Feb 28, 2015

@ghost ghost added the cla-not-required label Feb 28, 2015
@sornaks
Copy link
Author

sornaks commented Feb 28, 2015

/cc: @NTaylorMullen @dougbu

@@ -157,7 +159,7 @@ public TTagHelper CreateTagHelper<TTagHelper>() where TTagHelper : ITagHelper, n
/// </remarks>
public void StartWritingScope()
{
StartWritingScope(new StringWriter());
StartWritingScope(new StringCollectionTextWriter(Encoding.UTF8));
Copy link
Contributor

Choose a reason for hiding this comment

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

@dougbu do we have some app wide setting for encoding or is using UTF8 aok?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not aware of a system-wide setting. but in normal use RazorTextWriter is passed ViewContext.Writer.Encoding when created. AFAICT that value is always Encodings.UTF8EncodingWithoutBOM.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively use the Encoding from the current TextWriter - StartWritingScope(new StringCollectionTextWriter(Output.Encoding));

Copy link
Member

Choose a reason for hiding this comment

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

👍 to @pranavkm's suggestion

@NTaylorMullen
Copy link
Contributor

/// </summary>
/// <param name="writer">The <see cref="TextWriter"/> to which the output must be written to.</param>
/// <param name="copyableTextWriter">Contains the data which needs to be written to the output.</param>
public void WriteTo(TextWriter writer, ITextWriterCopyable copyableTextWriter)
Copy link
Member

Choose a reason for hiding this comment

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

as mentioned in aspnet/Razor#312, these methods must be named WriteLiteralTo because they perform no HTML encoding.

@dougbu
Copy link
Member

dougbu commented Mar 1, 2015

@@ -16,10 +15,10 @@ public class AutoLinkerTagHelper : TagHelper
var childContent = await context.GetChildContentAsync();

// Find Urls in the content and replace them with their anchor tag equivalent.
output.Content = Regex.Replace(
childContent,
output.Content.Append(Regex.Replace(
Copy link
Contributor

Choose a reason for hiding this comment

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

set

@NTaylorMullen
Copy link
Contributor

@sornaks should do Append when we used to do += and SetContent when we used to do =. Don't want to change that behavior

@NTaylorMullen
Copy link
Contributor

⌚ few more things, it's close

}
else
{
tagHelperContentWrapperTextWriter.Write(writer.ToString());
Copy link
Member

Choose a reason for hiding this comment

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

should also special-case the StringCollectionTextWriter created in StartTagHelperWritingScope. the ToString() here loses many of the advantages of the Razor changes.

Copy link
Member

Choose a reason for hiding this comment

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

What's the declared/runtime type of writer here? TextWriter.ToString() is not guaranteed to return the content - there can be trouble if the writer is replaceable.

We can leave this for a follow up as this assumption was present in the old code.

@dougbu
Copy link
Member

dougbu commented Mar 4, 2015

/// <summary>
/// The <see cref="TagHelperContent"/> which is wrapped.
/// </summary>
public TagHelperContent Content { 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.

Advantage of having this get/set vs constructor + immutable? Are there cases where you would create the writer at one time and then the content later?

Copy link
Author

Choose a reason for hiding this comment

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

No. Currently there are no cases like that. I can make this immutable.

@sornaks
Copy link
Author

sornaks commented Mar 4, 2015

Updated..

}
else
{
selected = encodedValues.Contains((await context.GetChildContentAsync()).GetContent());
Copy link
Contributor

Choose a reason for hiding this comment

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

two lines:

var childContent = await context.GetChildContentAsync();
selected = encodedValues.Contains(childContent.GetContent());

@NTaylorMullen
Copy link
Contributor

:shipit: once @dougbu is happy.

/// <summary>
/// Writes an <see cref="ITextWriterCopyable"/> to the <paramref name="writer"/>.
/// </summary>
/// <param name="writer">The <see cref="TextWriter"/> to which the <paramref name="copyableTextWriter"/> is written.</param>
Copy link
Member

Choose a reason for hiding this comment

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

nit: long line

@dougbu
Copy link
Member

dougbu commented Mar 4, 2015

:shipit: do not hold up submission for ToString -> GetContent nits (unless that's a 10 min. change)

@sornaks
Copy link
Author

sornaks commented Mar 4, 2015

Checked in. Thanks guys! 284eb9a

@sornaks sornaks closed this Mar 4, 2015
@sornaks sornaks deleted the Output branch March 5, 2015 17:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants