Refactor WriteAttribute \ AddHtmlAttribute #554
Conversation
| WriteStringLiteral(value.Value); | ||
| WriteParameterSeparator(); | ||
| Write(value.Location.AbsoluteIndex.ToString(CultureInfo.CurrentCulture)); | ||
| WriteEndMethodInvocation(false); | ||
| Write(value.Location.AbsoluteIndex.ToString(CultureInfo.InvariantCulture)); |
There was a problem hiding this comment.
No reason for generated code to rely on user culture.
There was a problem hiding this comment.
As mentioned later, this doesn't fit in this PR.
There was a problem hiding this comment.
Leaving as is. It's too small a change and has no user impact.
There was a problem hiding this comment.
Either this change has no user impact and isn't worth making or there's a scenario where the user's culture could result in compilation failures and we should be tracking it as an issue, making sure it the problem doesn't exist elsewhere, testing, ...
| .WriteParameterSeparator() | ||
| .Write(chunk.Start.AbsoluteIndex.ToString(CultureInfo.InvariantCulture)) | ||
| .WriteParameterSeparator() | ||
| .Write(chunk.Association.Length.ToString(CultureInfo.InvariantCulture)) |
There was a problem hiding this comment.
Shouldn't need the length at runtime.
There was a problem hiding this comment.
Really starting to think you're mixing an unrelated change into this PR. If there's a culture-related issue, file it separately.
There was a problem hiding this comment.
|
⌚ |
| @@ -467,15 +470,32 @@ private void RenderUnboundAttribute(string attributeName, Chunk attributeValueCh | |||
| // Dynamic attribute value should be run through the conditional attribute removal system. It's | |||
| // unbound and contains C#. | |||
|
|
|||
| // TagHelper attribute rendering is buffered by default. We do not want to write to the current | |||
| // writer. | |||
| var currentTargetWriter = _context.TargetWriterName; | |||
There was a problem hiding this comment.
Move these lines down to where it has effect where we do the _attributeCodeVisitor.Accept(attributeValueChunk);
|
To fulfill the purpose of the issue we should be conditionally generating the instrumented bits (value length, value offset etc.) based on instrumentation. ⌚ |
|
@NTaylorMullen The primary issue with conditionally generating these values is that we'll have to factor it in the page cache invalidation which is more work than it needs to be. Right now, we always generate it instrumented with the assumption that having no-op instrumentation methods and parameters is very cheap. I've updated the work item to reflect this design choice. |
pranavkm
force-pushed
the
prkrishn/177
branch
from
9d30f43
to
8697237
Compare
|
|
Fixes #177