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

Fix ModelExpression's in section directive blocks. #1615

Merged
merged 1 commit into from
Aug 16, 2017
Merged

Conversation

NTaylorMullen
Copy link

  • The DirectiveRemovalPass runs at DefaultFeatureOrder + 50 in order to allow the design time directive tokens pass to consume directive intermediate nodes; however, this has a side effect of leaving around the original @section nodes a bit too long. The ModelExpressionPass would see two references to the sections body (SectionIntermediateNode and DirectiveIntermediateNode) and then do its work twice. To combat this behavior I bumped the ModelExpressionPass order to be 50 points higher than the directive removal pass.
  • Added MVC tests for current and 1_X extensions.

#1614

Copy link
Contributor

@ajaybhargavb ajaybhargavb left a comment

Choose a reason for hiding this comment

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

Might be worth adding a comment in DirectiveRemovalOptimizationPass to explain the + 50 as well.
LGTM. We should later revisit other passes in this phase (maybe as part of https://github.com/aspnet/Razor/issues/1616) and make sure they are less fragile.

@@ -13,6 +13,8 @@ public class ModelExpressionPass : IntermediateNodePassBase, IRazorOptimizationP
{
private const string ModelExpressionTypeName = "Microsoft.AspNetCore.Mvc.ViewFeatures.ModelExpression";

public override int Order => DefaultFeatureOrder + 100;
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment here to explain the + 100

@rynowak
Copy link
Member

rynowak commented Aug 15, 2017

A high level suggestion I have to make this more flexible would be to add an annotation at the document level inside the model pass. That's something I considered when bringing up this feature anyway.

@NTaylorMullen
Copy link
Author

A high level suggestion I have to make this more flexible would be to add an annotation at the document level inside the model pass. That's something I considered when bringing up this feature anyway.

Like mark nodes that have already been handled by the ModelExpressionPass?

@rynowak
Copy link
Member

rynowak commented Aug 15, 2017

IIRC you said there was some trickiness about the ordering of passes wrt determining the model type. That's what I was referring to

@NTaylorMullen
Copy link
Author

IIRC you said there was some trickiness about the ordering of passes wrt determining the model type. That's what I was referring to

Ah, ya if we wanted to take that approach we could mark the document with an annotation stating the model type. Honestly wouldn't be that awful 😄

@NTaylorMullen
Copy link
Author

🆙 📅 to have the section directive pass move non-token section nodes from the directive intermediate node to the section intermediate node. This way there's no double reference of those nodes.

section.Children.Add(directive.Node.Children[i]);
directive.Node.Children.RemoveAt(i--);
Copy link
Member

Choose a reason for hiding this comment

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

It would be better to change this to a while loop. This is just.... an abomination.

Copy link
Member

Choose a reason for hiding this comment

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

#TaylorControlFlow

- Changed `SectionDirectivePass` to move non-token body nodes from the original `DirectiveIntermediateNode` to the `SectionIntermediateNode`. By doing this there's no longer dual references of `SectionIntermediateNode` bodies.
- Added MVC tests for current and 1_X extensions.

#1614
@NTaylorMullen NTaylorMullen merged commit db805eb into dev Aug 16, 2017
@NTaylorMullen NTaylorMullen deleted the nimullen/1614 branch August 16, 2017 17:25
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.

4 participants