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

Razor Pages Feedback - First impressions. Not good enough (yet). #6535

Closed
Bartmax opened this Issue Jul 12, 2017 · 22 comments

Comments

Projects
None yet
9 participants
@Bartmax

Bartmax commented Jul 12, 2017

TL;DR: I want to be able to customize URLs, Methods Names and the solution tree (folders/files) totally freely and also have better defaults.
Disclaimer: probably most stuff I point out is possible/configurable, I didn't spend huge amount of time, the way that was intuitive for me didn't work and wanted to give feedback, hence keyword is first impressions


@DamianEdwards talk about how this is a best fit for "feature folders" that I use in MVC and I agree that the configuration is kind of a clunky. but after 10 minutes of use I find it lacks some very important/key aspects I want to share with the team.

  1. _ViewImports, _Layout, _ViewStart, should not be on Pages root folder by default. This clutters the root folder.

  2. Partial/Shared are currently searched on Views folder and not Pages folder. This feels really weird. I think there's should be an abstraction on ViewLocator on where/which root folder it should search for views that is configurable so MVC can use Views and RazorPages can use Pages or something like that.

  3. Attribute Routing does not work ? I tried to move Index.cs* outside of Pages/ to something like Pages/Home and put [Route("")] at the cs file hoping to work but failed miserably. Maybe there's some way to do this ? It would be great if attribute routing works just as it does on mvc. Switching context shouldn't be that different.

  4. handlers and prefixed On methods: I would like to customize my urls/methods to my hearts intent. This is very critical feature for me.

  • The defaults are ugly. OnGet / OnPost, / OnGetAsync, etc. I would really liked it more to be Get / Post (like web api) and just be async/sync based on the return type (IActionResult (?) ).
  • I also hopped that I can use AttributeRouting here [Get("account")] to define the route. I don't like the custom handler thing that was implemented on Razor Pages at all.

At this point, the solution tree, the method names and the handler thing make this too subpar to feature folders on mvc I had to stop researching into pages. I really wished this works as expected 😞 I see lot of potential and I see how I could prefer it over MVC but right now I think it needs more work.

From my understanding, people that want/like feature folders (like me) do really care a lot on crafting methods names, routing names (urls), and specially folder structure (good defaults are very welcome), razor pages is failing on those 3 points at the moment.

This is not a rant, it's just constructive feedback on my first impressions. I really want Razor Pages to be a better development model than mvc! I do care and that's why I do post this issue.

Hope it helps!

p.s.: maybe consider a github repo for razor pages only ? if there's one sorry, couldn't find it.

@DamianEdwards

This comment has been minimized.

Show comment
Hide comment
@DamianEdwards

DamianEdwards Jul 12, 2017

Member

Thanks for the feedback. I'll try to address each of your points:

  1. _ViewImports.cshtml and _ViewStart.cshtml work the same way as they do with views, so they have to be hierarchical (you can't put them in Shared for views either). We could potentially change that in the future for both, but it works the same way today. _Layout can be wherever you like but I prefer it in the root. Each to their own.
  2. Yes, we don't search inside a Shared folder in the configured pages root by default, but you can change that. See #6534 (comment) for details. We landed on the side of following default conventions from page-focused frameworks rather than MVC's view search patterns (when working exclusively with pages), but we can revisit adding Shared as a search path by default in the next release.
  3. We made an implementation decision in this version to allow us the freedom to change the way we find and compile pages in the app in the future, and that decision prevented us from supporting attribute routes on PageModel classes. There's a trade-off to be made here given the way routing has to interplay with the dynamic nature of CSHTML files (we detect page files coming and going while the app is running). Today we scan for all pages up front when the app starts in order to discover their route information, but that doesn't involve compiling the page (yet) so we can't correctly determine its PageModel (at this stage) in order to discover an attribute route and thus update the route table. Other attributes (like filter attributes) work on the PageModel because they're evaluated at a later stage, after the pages have been compiled. In our testing, scanning the pages of a very large app (e.g. 1,000 pages) only takes a few hundred ms, so this might be something we can just not worry about in order to support this feature (which lots of people have asked for).
  4. You can change the handler method naming conventions. See @hishamco's blog post at http://www.hishambinateya.com/customize-razor-pages-handlers for a good example. The Async suffix is completely optional by default, so you can just not put it there. The way page handler dispatching works was inspired and intended to work well with how HTML5 formaction works, and you can easily state that you want the handler name to be passed by a segment in the route rather than the query string (@page "{handler?}", or do it globally with an IPageRouteConvention), because it all comes from routing ultimately. Again, we landed on making the handler name conventions follow page-focused patterns by default, rather than controller or Web API style naming. We can look at adding more conventions in the box by default in the future so switching is a simple one-liner in Startup.cs. Also, you can add a route in Startup that maps any given URL to a specific handler method on a specific page. You just can't specify it via an attribute due to the reasons above (again, we could revisit that in the next minor release).

Much of this is subjective (other than the attribute routing lifetime issue which I explained). I invite you to try using the approaches above to configure the Razor Pages conventions to your preferences.

P.S. Razor Pages is part of MVC, so we have no plan to make it its own repo.

Member

DamianEdwards commented Jul 12, 2017

Thanks for the feedback. I'll try to address each of your points:

  1. _ViewImports.cshtml and _ViewStart.cshtml work the same way as they do with views, so they have to be hierarchical (you can't put them in Shared for views either). We could potentially change that in the future for both, but it works the same way today. _Layout can be wherever you like but I prefer it in the root. Each to their own.
  2. Yes, we don't search inside a Shared folder in the configured pages root by default, but you can change that. See #6534 (comment) for details. We landed on the side of following default conventions from page-focused frameworks rather than MVC's view search patterns (when working exclusively with pages), but we can revisit adding Shared as a search path by default in the next release.
  3. We made an implementation decision in this version to allow us the freedom to change the way we find and compile pages in the app in the future, and that decision prevented us from supporting attribute routes on PageModel classes. There's a trade-off to be made here given the way routing has to interplay with the dynamic nature of CSHTML files (we detect page files coming and going while the app is running). Today we scan for all pages up front when the app starts in order to discover their route information, but that doesn't involve compiling the page (yet) so we can't correctly determine its PageModel (at this stage) in order to discover an attribute route and thus update the route table. Other attributes (like filter attributes) work on the PageModel because they're evaluated at a later stage, after the pages have been compiled. In our testing, scanning the pages of a very large app (e.g. 1,000 pages) only takes a few hundred ms, so this might be something we can just not worry about in order to support this feature (which lots of people have asked for).
  4. You can change the handler method naming conventions. See @hishamco's blog post at http://www.hishambinateya.com/customize-razor-pages-handlers for a good example. The Async suffix is completely optional by default, so you can just not put it there. The way page handler dispatching works was inspired and intended to work well with how HTML5 formaction works, and you can easily state that you want the handler name to be passed by a segment in the route rather than the query string (@page "{handler?}", or do it globally with an IPageRouteConvention), because it all comes from routing ultimately. Again, we landed on making the handler name conventions follow page-focused patterns by default, rather than controller or Web API style naming. We can look at adding more conventions in the box by default in the future so switching is a simple one-liner in Startup.cs. Also, you can add a route in Startup that maps any given URL to a specific handler method on a specific page. You just can't specify it via an attribute due to the reasons above (again, we could revisit that in the next minor release).

Much of this is subjective (other than the attribute routing lifetime issue which I explained). I invite you to try using the approaches above to configure the Razor Pages conventions to your preferences.

P.S. Razor Pages is part of MVC, so we have no plan to make it its own repo.

@DamianEdwards DamianEdwards added this to the Discussions milestone Jul 12, 2017

@hishamco

This comment has been minimized.

Show comment
Hide comment
@hishamco

hishamco Jul 12, 2017

Contributor

I'm totally agree with what @DamianEdwards mentioned before, and I have something to add:

Please take enough time to discover the RazorPages features, and I'm sure you will change your thoughts 😄

Contributor

hishamco commented Jul 12, 2017

I'm totally agree with what @DamianEdwards mentioned before, and I have something to add:

Please take enough time to discover the RazorPages features, and I'm sure you will change your thoughts 😄

@Bartmax

This comment has been minimized.

Show comment
Hide comment
@Bartmax

Bartmax Jul 12, 2017

@DamianEdwards @hishamco Thanks for the detailed and useful response and information.

I certainly will be investing more time in Razor Pages and play with it more in depth by it's rules and not mines ;) and see how it goes.

Looks like I can achieve most of my needs with simple configuration. 🎉 !

also makes me very happy that point 3 and point 4 may be addressed in the future if/when Attributes on PageModel can be used. I'll let you know how it goes!

Bartmax commented Jul 12, 2017

@DamianEdwards @hishamco Thanks for the detailed and useful response and information.

I certainly will be investing more time in Razor Pages and play with it more in depth by it's rules and not mines ;) and see how it goes.

Looks like I can achieve most of my needs with simple configuration. 🎉 !

also makes me very happy that point 3 and point 4 may be addressed in the future if/when Attributes on PageModel can be used. I'll let you know how it goes!

@pranavkm

This comment has been minimized.

Show comment
Hide comment
@pranavkm

pranavkm Jul 19, 2017

Member

@Bartmax I wrote a sample that shows how you'd use page application model to configure the naming of handlers.

Member

pranavkm commented Jul 19, 2017

@Bartmax I wrote a sample that shows how you'd use page application model to configure the naming of handlers.

@hishamco

This comment has been minimized.

Show comment
Hide comment
@hishamco

hishamco Jul 19, 2017

Contributor

Thanks @pranavkm for the sample that you provide, it would be nice if we add the handler to the route by default instead of adding it in the page itself @page "{handler?}"

Contributor

hishamco commented Jul 19, 2017

Thanks @pranavkm for the sample that you provide, it would be nice if we add the handler to the route by default instead of adding it in the page itself @page "{handler?}"

@pranavkm

This comment has been minimized.

Show comment
Hide comment
@pranavkm

pranavkm Jul 20, 2017

Member

@hishamco it's trivial to add this globally using a page route model convention:

using Microsoft.AspNetCore.Mvc.ApplicationModels;
...

.AddMvc()
.AddRazorPagesOptions(options =>
{
    options.Conventions.AddFolderRouteModelConvention("/", model =>
    {
        foreach (var selector in model.Selectors)
        {
            var attributeRouteModel = selector.AttributeRouteModel;
            attributeRouteModel.Template = AttributeRouteModel.CombineTemplates(attributeRouteModel.Template, "{handler?}");
        }
    });
});
Member

pranavkm commented Jul 20, 2017

@hishamco it's trivial to add this globally using a page route model convention:

using Microsoft.AspNetCore.Mvc.ApplicationModels;
...

.AddMvc()
.AddRazorPagesOptions(options =>
{
    options.Conventions.AddFolderRouteModelConvention("/", model =>
    {
        foreach (var selector in model.Selectors)
        {
            var attributeRouteModel = selector.AttributeRouteModel;
            attributeRouteModel.Template = AttributeRouteModel.CombineTemplates(attributeRouteModel.Template, "{handler?}");
        }
    });
});
@hishamco

This comment has been minimized.

Show comment
Hide comment
@hishamco

hishamco Jul 20, 2017

Contributor

Thanks a lot @pranavkm, but the AddFolderRouteModelConvention() in your case above it will add the handler for root pages if I'm not wrong

I'm not sure if it will cover the pages that available in the nested folders?!!

Contributor

hishamco commented Jul 20, 2017

Thanks a lot @pranavkm, but the AddFolderRouteModelConvention() in your case above it will add the handler for root pages if I'm not wrong

I'm not sure if it will cover the pages that available in the nested folders?!!

@pranavkm

This comment has been minimized.

Show comment
Hide comment
@pranavkm

pranavkm Jul 20, 2017

Member

AddFolderRouteModelConvention is invoked for all files that belong to the specified directory (recursively).

Member

pranavkm commented Jul 20, 2017

AddFolderRouteModelConvention is invoked for all files that belong to the specified directory (recursively).

@hishamco

This comment has been minimized.

Show comment
Hide comment
@hishamco

hishamco Jul 20, 2017

Contributor

I think I will write another blog post handlers & routing with the built-in model conventions

thanks again ..

Contributor

hishamco commented Jul 20, 2017

I think I will write another blog post handlers & routing with the built-in model conventions

thanks again ..

@scottsauber

This comment has been minimized.

Show comment
Hide comment
@scottsauber

scottsauber Jul 21, 2017

I'm trying out Razor Pages, as I prepare for an ASP.NET Core 2 talk (which Razor Pages is gonna take up ~25 of the 75 minutes), and these are my thoughts:

  • +1 for attribute routes. I think it's a little strange that the "{handler?}" route token is on the Page itself and not the PageModel. I know that you can probably just avoid using that if Attribute Routing lands in Razor Pages and you'll just configure it on the PageModel, but it still seems a bit strange.
  • The inline functions and stuff on the docs has to go, ESPECIALLY when it's in the beginning of the article. My co-workers took a 2 minute look at Razor Pages by visiting those docs and gave it a big ol' NOPE, because of that. I'm now fighting an uphill battle to give Razor Pages a second look. I would just take the inline stuff out entirely, but at least put it at the end or something, not the beginning. I doubt many people will put their PageModel directly on their view page in practice. It also doesn't help that some of the demos I've seen by MS employees also open with this approach or promote Razor Pages as "for simple things." I think this is a mistake. I won't be showing the functions directive at all in my talk.
  • I haven't quite locked in on where I've landed on this yet, but I feel like having a separate View Model class and hanging that off of PageModel feels right (especially for larger views) to avoid cluttering up the PageModel with a bunch of props. I'll be curious what patterns crop up in the community.

Otherwise - I love the Page focused approach. Seems like it's much harder to end up with a big ball of mud than Controllers with too many actions. 👍

FWIW - I also was converting MVC Pages/Controllers to Razor Pages and took me about 4-5 minutes per Action (including GET and POST). Love how smooth a lot of the code comes over.

scottsauber commented Jul 21, 2017

I'm trying out Razor Pages, as I prepare for an ASP.NET Core 2 talk (which Razor Pages is gonna take up ~25 of the 75 minutes), and these are my thoughts:

  • +1 for attribute routes. I think it's a little strange that the "{handler?}" route token is on the Page itself and not the PageModel. I know that you can probably just avoid using that if Attribute Routing lands in Razor Pages and you'll just configure it on the PageModel, but it still seems a bit strange.
  • The inline functions and stuff on the docs has to go, ESPECIALLY when it's in the beginning of the article. My co-workers took a 2 minute look at Razor Pages by visiting those docs and gave it a big ol' NOPE, because of that. I'm now fighting an uphill battle to give Razor Pages a second look. I would just take the inline stuff out entirely, but at least put it at the end or something, not the beginning. I doubt many people will put their PageModel directly on their view page in practice. It also doesn't help that some of the demos I've seen by MS employees also open with this approach or promote Razor Pages as "for simple things." I think this is a mistake. I won't be showing the functions directive at all in my talk.
  • I haven't quite locked in on where I've landed on this yet, but I feel like having a separate View Model class and hanging that off of PageModel feels right (especially for larger views) to avoid cluttering up the PageModel with a bunch of props. I'll be curious what patterns crop up in the community.

Otherwise - I love the Page focused approach. Seems like it's much harder to end up with a big ball of mud than Controllers with too many actions. 👍

FWIW - I also was converting MVC Pages/Controllers to Razor Pages and took me about 4-5 minutes per Action (including GET and POST). Love how smooth a lot of the code comes over.

@DamianEdwards

This comment has been minimized.

Show comment
Hide comment
@DamianEdwards

DamianEdwards Jul 28, 2017

Member

Thanks for the feedback folks. I logged a bunch of issues to cover these items (and some others) which I hope to get done as part of the next minor release:

Member

DamianEdwards commented Jul 28, 2017

Thanks for the feedback folks. I logged a bunch of issues to cover these items (and some others) which I hope to get done as part of the next minor release:

@Bartmax

This comment has been minimized.

Show comment
Hide comment
@Bartmax

Bartmax Aug 29, 2017

How can I prevent Razor Page to accept Post request when those are not defined on PageModel ?
It's very weird default behavior, as I see, post is accepted, no method will be executed and Page.cshtml will be loaded, since the Get method won't get executed, any initialization data will be empty (or null) leading to easily get exceptions instead of Not Found or Method not allowed status code.

Sample code with commented workaround.

public class IndexModel : PageModel
{
    private readonly ApplicationDbContext context;
    public IndexModel(ApplicationDbContext context)
    {
        this.context = context;
    }

    public IList<RestoreData> Restores { get; private set; } = new List<RestoreData>();

    public async Task OnGet()
    {
        Restores = await context.Restores
            .AsNoTracking()
            .ToListAsync();
    }

    //public IActionResult OnPost()
    //{
    //    return NotFound();
    //}
}

Bartmax commented Aug 29, 2017

How can I prevent Razor Page to accept Post request when those are not defined on PageModel ?
It's very weird default behavior, as I see, post is accepted, no method will be executed and Page.cshtml will be loaded, since the Get method won't get executed, any initialization data will be empty (or null) leading to easily get exceptions instead of Not Found or Method not allowed status code.

Sample code with commented workaround.

public class IndexModel : PageModel
{
    private readonly ApplicationDbContext context;
    public IndexModel(ApplicationDbContext context)
    {
        this.context = context;
    }

    public IList<RestoreData> Restores { get; private set; } = new List<RestoreData>();

    public async Task OnGet()
    {
        Restores = await context.Restores
            .AsNoTracking()
            .ToListAsync();
    }

    //public IActionResult OnPost()
    //{
    //    return NotFound();
    //}
}
@Ibro

This comment has been minimized.

Show comment
Hide comment
@Ibro

Ibro Aug 29, 2017

Any updates on the Scott's comment regarding docs? Definitely agree on this one.

The inline functions and stuff on the docs has to go, ESPECIALLY when it's in the beginning of the article. My co-workers took a 2 minute look at Razor Pages by visiting those docs and gave it a big ol' NOPE, because of that. I'm now fighting an uphill battle to give Razor Pages a second look. I would just take the inline stuff out entirely, but at least put it at the end or something, not the beginning. I doubt many people will put their PageModel directly on their view page in practice. It also doesn't help that some of the demos I've seen by MS employees also open with this approach or promote Razor Pages as "for simple things." I think this is a mistake. I won't be showing the functions directive at all in my talk.

@DamianEdwards anything you can do?

Ibro commented Aug 29, 2017

Any updates on the Scott's comment regarding docs? Definitely agree on this one.

The inline functions and stuff on the docs has to go, ESPECIALLY when it's in the beginning of the article. My co-workers took a 2 minute look at Razor Pages by visiting those docs and gave it a big ol' NOPE, because of that. I'm now fighting an uphill battle to give Razor Pages a second look. I would just take the inline stuff out entirely, but at least put it at the end or something, not the beginning. I doubt many people will put their PageModel directly on their view page in practice. It also doesn't help that some of the demos I've seen by MS employees also open with this approach or promote Razor Pages as "for simple things." I think this is a mistake. I won't be showing the functions directive at all in my talk.

@DamianEdwards anything you can do?

@DamianEdwards

This comment has been minimized.

Show comment
Hide comment
@DamianEdwards

DamianEdwards Aug 29, 2017

Member

I thought the docs had already been updated to remove or at the very least downplay those examples. I'll check again.

Member

DamianEdwards commented Aug 29, 2017

I thought the docs had already been updated to remove or at the very least downplay those examples. I'll check again.

@DamianEdwards

This comment has been minimized.

Show comment
Hide comment
@DamianEdwards

DamianEdwards Aug 29, 2017

Member

@Bartmax you could likely do that with a page filter by detecting the incoming verb has no matching handler, and then shortcutting execution. I don't necessarily agree that pages without handlers for the incoming verb should not run however, but this does seem to be a vote for adding support for an initialization method on pages, just runs before all handlers. Again, that could be prototyped via a filter.

Member

DamianEdwards commented Aug 29, 2017

@Bartmax you could likely do that with a page filter by detecting the incoming verb has no matching handler, and then shortcutting execution. I don't necessarily agree that pages without handlers for the incoming verb should not run however, but this does seem to be a vote for adding support for an initialization method on pages, just runs before all handlers. Again, that could be prototyped via a filter.

@Bartmax

This comment has been minimized.

Show comment
Hide comment
@Bartmax

Bartmax Aug 29, 2017

I don't necessarily agree that pages without handlers for the incoming verb should not run

Would you mind to expand on this please? I see how GET and HEAD action are a nice feature to work without a method defined on the PageModel but I fail to see how or why this could be useful for a POST verb (and put, patch, etc. Any non-indepotent verb for that matter).

Bartmax commented Aug 29, 2017

I don't necessarily agree that pages without handlers for the incoming verb should not run

Would you mind to expand on this please? I see how GET and HEAD action are a nice feature to work without a method defined on the PageModel but I fail to see how or why this could be useful for a POST verb (and put, patch, etc. Any non-indepotent verb for that matter).

@Code-DJ

This comment has been minimized.

Show comment
Hide comment
@Code-DJ

Code-DJ Sep 4, 2017

Is there a setting to have runtime compilation when using PageModel?

Code-DJ commented Sep 4, 2017

Is there a setting to have runtime compilation when using PageModel?

@hishamco

This comment has been minimized.

Show comment
Hide comment
@hishamco
Contributor

hishamco commented Sep 4, 2017

@Code-DJ

This comment has been minimized.

Show comment
Hide comment
@Code-DJ

Code-DJ Sep 4, 2017

@hishamco just to be sure - In the webforms WebSite project, you could deploy both .aspx and .aspx.cs to the server without requiring pre-compilation. This meant that there was not need to deploy DLL to the bin folder when you made changes to your .aspx.cs files. Having to deploy DLLs caused a 1-2 second freeze of the app.

While I like the compilation/test steps during development for .cshtml.cs files. I wanted to see if there was a way to avoid pre-compilation for deployment. It's not a big deal, just wanted to find out if there was an option. Thanks!

Code-DJ commented Sep 4, 2017

@hishamco just to be sure - In the webforms WebSite project, you could deploy both .aspx and .aspx.cs to the server without requiring pre-compilation. This meant that there was not need to deploy DLL to the bin folder when you made changes to your .aspx.cs files. Having to deploy DLLs caused a 1-2 second freeze of the app.

While I like the compilation/test steps during development for .cshtml.cs files. I wanted to see if there was a way to avoid pre-compilation for deployment. It's not a big deal, just wanted to find out if there was an option. Thanks!

@hishamco

This comment has been minimized.

Show comment
Hide comment
@hishamco

hishamco Sep 4, 2017

Contributor

@Code-DJ I think it would be nice if you create a new issue to track it easily and listen from the other folks

Contributor

hishamco commented Sep 4, 2017

@Code-DJ I think it would be nice if you create a new issue to track it easily and listen from the other folks

@Code-DJ

This comment has been minimized.

Show comment
Hide comment
@Code-DJ

Code-DJ commented Sep 4, 2017

Thanks @hishamco created #6760

@hhvdblom

This comment has been minimized.

Show comment
Hide comment
@hhvdblom

hhvdblom Nov 6, 2017

Since Nadella is on board I don't jump in to easily anymore. Fool me once shame on you. Fool me twice, shame on me. Invested a lot in WPF for Mobile. Saw lots of things get dumped from Microsoft. Haha, Microsoft Software starts to look like Open Source, no backwards compatability, support etc.

hhvdblom commented Nov 6, 2017

Since Nadella is on board I don't jump in to easily anymore. Fool me once shame on you. Fool me twice, shame on me. Invested a lot in WPF for Mobile. Saw lots of things get dumped from Microsoft. Haha, Microsoft Software starts to look like Open Source, no backwards compatability, support etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment