Skip to content
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

[Routing] [Mvc] Support RoutePattern required values in matcher and link generator #4245

Merged
merged 2 commits into from
Dec 3, 2018

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Nov 27, 2018

  • DFA matcher uses required values to denormalize routes
  • Link generator uses required values to calculate route precedence
  • Obsolete IRouteValuesAddressMetadata
  • React in MVC to provide required values with RoutePattern in datasource

re: aspnet/Routing#914 and aspnet/Mvc#8731

@JamesNK JamesNK requested a review from rynowak November 27, 2018 04:43
@JamesNK
Copy link
Member Author

JamesNK commented Nov 27, 2018

@ryanbrandenburg @natemcmaster I just want to double check with you guys whether there is anything new or different when creating PRs in this post-mondo world. Any tricks or tips?

@JamesNK
Copy link
Member Author

JamesNK commented Nov 27, 2018

@ryanbrandenburg @natemcmaster Do feature branches work the same way in the new world?

I need to make updates to MVC with this PR's changes. Can that happen while MVC is non-mondo, or should it wait until MVC is migrated?

@JamesNK JamesNK added the breaking-change This issue / pr will introduce a breaking change, when resolved / merged. label Nov 27, 2018
@JamesNK
Copy link
Member Author

JamesNK commented Nov 27, 2018

@mkArtakMSFT FYI the breaking change here is IRouteValuesAddressMetadata is no longer used for required values. It has moved to RoutePattern.RequiredValues.

IRouteValuesAddressMetadata was introduced in 2.2 and I doubt anyone would use it beside us.

Edit:
Additional breaking change in this PR around MVC link generation. More detail here - #4212

@natemcmaster
Copy link
Contributor

We we're planning on getting MVC merged to this repo today so that you would have been able to make both changes at the same time. Unfortunately, other stuff got in the way so merging MVC got pushed to tomorrow. Once it is in, you can make the changes to routing and MVC projects in the same branch.

@JamesNK
Copy link
Member Author

JamesNK commented Nov 27, 2018

Ok, I'll wait until tomorrow.

Question about cross-solution changes: Under the routing directory there is still a build dir with a dependencies file in it. If I update the MVC solution and Routing solution at the same time, how do the Routing changes get used in MVC immediately?

@natemcmaster
Copy link
Contributor

how do the Routing changes get used in MVC immediately?

The dependencies file in src/*/build/ will eventually go away completely. That file is only used if you run src/*/build.cmd scripts. When the root-level build.cmd script is invoked, it does some fancy stuff generate a props file which ensures package versions are aligned.

https://github.com/aspnet/AspNetCore/blob/a0d1b8882b35820cb38e4683dc9b282d5a3b2017/build/RepositoryBuild.targets#L83

This is why all src/*/build/dependencies.props files are required to have this import:
https://github.com/aspnet/AspNetCore/blob/a0d1b8882b35820cb38e4683dc9b282d5a3b2017/src/AuthSamples/build/dependencies.props#L75

FYI Phase 1 of mondo-repo was to get all source in the same place. Phase 2 is #4246 (which has already started) which should make it even easier to make changes to multiple areas of the product at the same time.

@JamesNK
Copy link
Member Author

JamesNK commented Nov 27, 2018

Thanks for the info, I'm looking forward to phase 2!

For this PR, what do I need to do today for cross-solution changes? 😸 @rynowak wants to build on top of these changes and is blocked until we can get this merged

rynowak
rynowak previously approved these changes Nov 27, 2018
Copy link
Member

@rynowak rynowak left a comment

Choose a reason for hiding this comment

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

This looks like what I remember from the aspnet/Routing PR 👍

@JamesNK
Copy link
Member Author

JamesNK commented Nov 28, 2018

@rynowak Still blocked until #4273 is merged. Fingers crossed I can get it in tomorrow

@JamesNK JamesNK force-pushed the feature/routing-required-values branch from 7b054ba to 6ca735a Compare November 28, 2018 20:59
@JamesNK JamesNK added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Nov 28, 2018
@JamesNK JamesNK changed the title [Routing] Support RoutePattern required values in matcher and link generator [Routing] [Mvc] Support RoutePattern required values in matcher and link generator Nov 28, 2018
@JamesNK JamesNK force-pushed the feature/routing-required-values branch from b667708 to 38a88a7 Compare November 29, 2018 00:57
@JamesNK JamesNK dismissed rynowak’s stale review November 29, 2018 01:00

Added MVC reaction

@JamesNK
Copy link
Member Author

JamesNK commented Nov 29, 2018

🆙 📅 @rynowak

It builds. Tests fail but I believe they were already failing.

@@ -323,7 +323,7 @@ public async Task PagesInAreas_CanGenerateLinksToControllersAndPages()
var expected =
@"<a href=""/Accounts/Manage/RenderPartials"">Link inside area</a>
<a href=""/Products/List/old/20"">Link to external area</a>
<a href=""/Accounts"">Link to area action</a>
<a href="""">Link to area action</a>
Copy link
Member

Choose a reason for hiding this comment

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

I think I left this comment as my only feedback on the aspnet/Mvc PR.... why not change the test to do something valid?

I'm not sure I care a whole ton about this part because I think we will have to restore the 2.1/2.2 behaviour. I think it's fine to do that after this PR.

@JamesNK JamesNK force-pushed the feature/routing-required-values branch from 38a88a7 to 38ae814 Compare December 3, 2018 21:39
@JamesNK JamesNK merged commit f157b78 into master Dec 3, 2018
@JamesNK JamesNK deleted the feature/routing-required-values branch December 6, 2018 00:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates breaking-change This issue / pr will introduce a breaking change, when resolved / merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants