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

Update razor-pages-convention-features.md #6173

Closed
user135711 opened this issue May 2, 2018 · 8 comments
Closed

Update razor-pages-convention-features.md #6173

user135711 opened this issue May 2, 2018 · 8 comments
Assignees
Labels

Comments

@user135711
Copy link
Contributor

user135711 commented May 2, 2018

Three bugs:
1)Navigate to the sample app localhost:5000/TheContactPage
The index page is loaded instead of localhost:5000/Contact when the optional route is left off with the TheContact. I stepped through the call stack starting from the loading of RazorPages from the pages directory and haven't found the point where the index is loaded yet so I don't know if this is a doc or core bug.

2)Navigate to the sample app localhost:5000/TheContactPage/TextValue
Mouse over the links in the top and you will see that every link has been replaced with the TheContactPage appended to every link.

A confusing point in the article: The notes all indicate that the order property determines which route comes first {globalTemplate?} vs {aboutTemplate?}. This wording makes it seem like the order of directories /GlobalRouteValue/OtherPagesRouteValue was determined by this number but stepping through the sourcelink provided .NET CORE source code shows that the order of the route order is determined by the order that options.Conventions.Add are called in the startup. This is because the model.Selectors.Add in the sample adds a new selector attribute route model template on top of the one that already exists. So each route added creates a route plus the old ie 1,2, 1/2, 3, 1/3, 1/2/3, 1/4, 2/4, etc... The example happened to create a conflict by appending templates rather than adding a new template and the selection of the template when one value is provided then gets resolved between {globalTemplate?} vs {aboutTemplate?} using the order. I think you could make this clear just by adding one line at the top note saying the order of routes is determined by the template. You can see an example by switching the call order in startup between snippets 1 and 3 options.Conventions.AddFolderRouteModelConvention("/OtherPages" and options.Conventions.Add(new GlobalTemplatePageRouteModelConvention() and then navigating to localhost:5000/OtherPages/Page1/GlobalRouteValue/OtherPagesRouteValue and observe that the html now displays
"Route data for 'globalTemplate' was provided: OtherPagesRouteValue
Route data for 'otherPagesTemplate' was provided: GlobalRouteValue"
in contradiction with the note stating that the order property decided the route order "The Order property for the AttributeRouteModel is set to 1. This ensures that the template for {globalTemplate?} (set earlier in the topic) is given priority for the first route data value position". Here there was no conflict between equally specific routes and was determined by the calls to the order.

@guardrex guardrex self-assigned this May 2, 2018
@guardrex
Copy link
Collaborator

guardrex commented May 5, 2018

😅 Finally caught up! (just kidding ... I'm not even close to being caught up ... just felt nice to type that out). Anyway ... time to put an 👁️ on this. Let's take them each in turn ...

Issue 1

/TheContactPage is requested without a route param, and the Index page is loaded.

I see what you mean. It wasn't part of the testing to test without the route param, since I thought that the optional mark (?) on {text?} ...

options.Conventions.AddPageRoute("/Contact", "TheContactPage/{text?}");

... combined with the optional param in the page ...

@page "{text?}"

... would ✨ Just Work!™️ ✨

Hitting up /Contact/TextValue DOES load the Contact page with the correct param. Calling /Contact also loads the Contact page without it. It seems the optional route param is working for RP generally but failing specifically for the convention.

Attempting to declare two conventions FAILS ...

options.Conventions.AddPageRoute("/Contact", "TheContactPage");
options.Conventions.AddPageRoute("/Contact", "TheContactPage/{text}");

Yes, it seems broken to me. We need a consultation with Dr. @pranavkm 👨‍⚕️ on how this scenario is designed to work or if it's a real bug.

Issue 2

Mouse over the links in the top and you will see that every link has been replaced with the TheContactPage appended to every link.

After selecting the Contact page link without a route param (the URL changes to /TheContactPage but the Index page loads as described in Issue 1). Also ... "TheContactPage" is appended to the other links ...

<ul class="nav navbar-nav">
    <li><a href="/Index/TheContactPage">Home</a></li>
    <li>
        <a href="#" class="dropdown-toggle" data-toggle="dropdown">Other Pages <b class="caret"></b></a>
        <ul class="dropdown-menu multi-level">
            <li><a href="/OtherPages/Page1/TheContactPage">Page 1</a></li>
            <li><a href="/OtherPages/Page2/TheContactPage">Page 2</a></li>
            <li><a href="/OtherPages/Page3/TheContactPage">Page 3</a></li>
        </ul>
    </li>
    <li><a href="/About/TheContactPage">About</a></li>
    <li><a href="/TheContactPage">Contact</a></li>
</ul>

@pranavkm ☝️ 🐞 bug?

When a route param is provided (/TheContactPage/TextValue), the links seem ok ...

  • Other Pages (drop-down) ... but that's an href="#". If changed to href="/SomethingElse" the mouseover is correct for /SomethingElse. That's not a navigable link anyway.
  • Contact ... the mouseover is for the same destination. I think this is normal behavior. The reason being that the visitor was just sent to this page with this route param. Reloading should generate the same response behavior. If they then navigate away, the nav bar link for the Contact page goes back to /TheContactPage (no route param).

Issue 3

the order of the route order is determined by the order that options.Conventions.Add are called in the startup. This is because the model.Selectors.Add in the sample adds a new selector attribute route model template on top of the one that already exists.

adding one line at the top note saying the order of routes is determined by the template. You can see an example by switching the call order in startup between snippets 1 and 3 options.Conventions.AddFolderRouteModelConvention("/OtherPages" and options.Conventions.Add(new GlobalTemplatePageRouteModelConvention() and then navigating to localhost:5000/OtherPages/Page1/GlobalRouteValue/OtherPagesRouteValue and observe that the html now displays

Route data for 'globalTemplate' was provided: OtherPagesRouteValue
Route data for 'otherPagesTemplate' was provided: GlobalRouteValue

Yes! Great points. The topic is lacking a clear explanation on order, and the sample probably isn't setting the Order prop correctly. [EDIT: Before I confirm what's going on with the Order settings, I want to get the routes in order and look 👀. I'll do that and address any sample problems when I see what's what with the assigned order.]

I'll work on this for an upcoming PR to fix what we've discussed here (after we get engineering feedback on the first two issues). It would also be nice to add a bit of code that outputs the routes in order. That would be a handy little piece of troubleshooting code that a dev could use to sort out why routing behavior isn't quite what they expected when working with the conventions. I'll see if something like that can be rolled into the updates.

Notes on ordering for my reference: https://docs.microsoft.com/aspnet/core/mvc/controllers/routing#ordering-attribute-routes

btw ... At the time the topic was written, I think they were set to 1 because of this remark ...

Setting a route using Order = 1 will run after default route ordering.

@guardrex guardrex added the bug label May 5, 2018
@user135711
Copy link
Contributor Author

user135711 commented May 6, 2018

@guardrex See the link for #2. The contact page appears correct because it's a link to the default index with TheContactPage appended. Each of the links will still load the correct link but the url link and the url in the address bar once clicked have an erroneous TheContactPage, that seems to be tolerated by the routing.
contact

@guardrex
Copy link
Collaborator

guardrex commented May 6, 2018

Sorry ... I didn't understand the repro. Yes, I see it now. I updated "Issue 2" and show the markup generated.

@guardrex
Copy link
Collaborator

guardrex commented Sep 4, 2018

@user135711 I'm working on this now. I ask the MVC/RP engineers about the behavior on aspnet/Mvc#8385. After hearing back, I can proceed. They'll probably either say "by-design" or "bug." After Issues 1 and 2 are addressed here, I'll take a look at Issue 3.

@user135711
Copy link
Contributor Author

@guardrex thanks for letting me know, I subscribed to the other thread.

@guardrex
Copy link
Collaborator

I have enough info now to work a sample update for Issues 1 and 2. There's a conflict between scenarios in the sample app.

This is still on this sprint, so I hope to get to it shortly. I'll post an update (or a PR) shortly-ish†.

†Idk exactly when ... 🏃 😅 crazy busy these days with 2.2 issues. I'll get this asap.

@user135711
Copy link
Contributor Author

@guardrex yes, I watched the solution as it unfolded :)

@guardrex
Copy link
Collaborator

guardrex commented Sep 12, 2018

Best intentions at least. I don't want multiple samples for any of these topics (if possible). It's unlikely that a given dev would be required to implement several of these in one app, but it's best for the repo if we restrict sample proliferation. I suspect that when I move the /TheContactPage page route UP! in the order that it will match and the whole sample will work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants