Add feature #398

[maxbanas]

I'm not able to run tests for net5.0, but tests for netcoreapp2.0 and netcoreapp3.1 are passing.

[maxbanas]

From previous conversation:

One review item: master...maxbanas:feature/398diff-faaf482eb73b368fd3c2aeb32459b8cf25da987a137c77fff3145e741591b520R298

For some reason this link doesn't work for me, but I think I fixed the issue.

The only incremental improvement I could suggest is doing something where we compute the closest matching template key.

I agree this would be a nice feature. Would an exception would still be thrown, but with an addition to the message like "did you mean Closest.Match.cshtml?"?

Or handling the special case where an EmbeddedResource Project has.... zero embedded resources found? (In this case, it would make sense to maybe include a link to Microsoft Docs on adding cshtml files as embedded resources, noting the default auto-globbing rule for cshtml files is Content https://docs.microsoft.com/en-us/aspnet/core/razor-pages/sdk?view=aspnetcore-5.0#overview)

I thought about this, and think it would be helpful, but I tried to keep my additions to RazorTemplateCompiler (where the exception is thrown) generic to any project type, and didn't want to make things overcomplicated (e.g. having each project implementation create their own exception message). But I suppose since embedded resource (and file system) projects are built into RazorLight (and probably what 99% of people use), it would make sense to add a more customized message when no templates are found at all for built-in project types.

In the case of perhaps having a generic no templates found at all exception, we might want the error message to mention this can happen if people render raw strings (as there is no template cache IIRC).

Do you mean when RazorLightEngine.CompileRenderStringAsync is used? I'm not sure about the cache, but I was thinking that the key and content ended up getting stored in a dict: RazorLightOptions.DynamicTemplates. This is where I'm getting the known keys for KnownDynamicTemplateKeys on TemplateNotFoundException.

[jzabroski]

For some reason this link doesn't work for me, but I think I fixed the issue.

👍

I agree this would be a nice feature. Would an exception would still be thrown, but with an addition to the message like "did you mean Closest.Match.cshtml?"?

Yes. That is how I do it in my clients projects for FluentMigrator, another project I maintain. While I don't have my EmbeddedResourceUtility public, since someone else at my client wrote a good chunk of it, the core idea is that I find the closest matching suffix, by first seeing if any namespaces (folders) are similar. I wish I could just copy-paste that code here, but for legal reasons, I cannot.

I thought about this, and think it would be helpful, but I tried to keep my additions to RazorTemplateCompiler (where the exception is thrown) generic to any project type, and didn't want to make things overcomplicated (e.g. having each project implementation create their own exception message).

Good point. Don't put it in the compiler layer. Let's move it to a validation layer when we first construct the project system.

But I suppose since embedded resource (and file system) projects are built into RazorLight (and probably what 99% of people use), it would make sense to add a more customized message when no templates are found at all for built-in project types.

I think we could add an Option in DI similar to how WebHost used to work pre-.netcoreapp3.0:

                .UseDefaultServiceProvider((context, options) =>
                {
                    options.ValidateScopes = context.HostingEnvironment.IsDevelopment();
                });

This would require knowing from a serviceCollection whether we are in IsDevelopment mode or not.

[jzabroski]

for .netcoreapp3.0 and up, We can know whether IsDevelopment is true by injecting a generic IHostEnvironment: https://docs.microsoft.com/en-us/dotnet/api/microsoft.extensions.hosting.ihostenvironment?view=dotnet-plat-ext-5.0

Would need to research how to do it in netstandard

[jzabroski]

@maxbanas Your tests (and the full regression suite) passed on all TFMs. Thanks. I think the other enhancements we just discussed should go into a separate PR.

[maxbanas]

Good point. Don't put it in the compiler layer. Let's move it to a validation layer when we first construct the project system.

This sound good, but what about the case for using purely dynamic templates (no embedded templates), which aren't added ahead of time? For example, this works now:

var engine = new RazorLightEngineBuilder()
  .UseEmbeddedResourcesProject(typeof(Program))
  .UseMemoryCachingProvider()
  .Build();
var result = await engine.CompileRenderStringAsync("myKey", content, myModel);

I guess we'll just need to play around with this idea a bit in a future pr.

[jzabroski]

It seems a bit odd to me to UseEmbeddedResourcesProject(typeof(Program)) and does not have at least one EmbeddedResource. However, in that scenario, they just don't respect EnableDebugMode and ignore the IHostingEnvironment. That would allow the trivial case where a brand new project should let you build the RazorLightEngine object via DI and call some basic stuff without it failing.

[maxbanas]

It seems a bit odd to me to UseEmbeddedResourcesProject(typeof(Program)) and does not have at least one EmbeddedResource.

I think it's weird too, but if users just want to render templates from memory/strings, isn't a project still required? From the readme:

var razorEngine = new RazorLightEngineBuilder()
                .UseEmbeddedResourcesProject(typeof(AnyTypeInYourSolution)) // exception without this (or another project type)
                .UseMemoryCachingProvider()
                .Build();

However, in that scenario, they just don't respect EnableDebugMode and ignore the IHostingEnvironment. That would allow the trivial case where a brand new project should let you build the RazorLightEngine object via DI and call some basic stuff without it failing

Throwing an exception when debug mode is on (but not throwing one when it's off, and all other things are equal) seems strange to me. I think it would really be nice to have logging available here, and in a few other situations like here where we could just log warnings when debug mode is enabled, rather than changing actual behavior in debug vs non-debug.

[maxbanas]

I just realized that RazorLightDependencyBuilder existed. I'm not entirely sure what its purpose is, and when/why you would use this vs. the other AddRazorLight extension method. It looks like this class has/had similar methods to RazorLightEngineBuilder, but is missing a few methods including EnableDebugMode(), which was added by this pr. Is RazorLightDependencyBuilder actually used, and if so, does it need to be updated to include these missing methods?

[Kissaki]

It would be great if PR titles could be descriptive.

Release notes point to milestones. Milestones list PR without descriptive titles. PR mentions ticket in title without context or description. Ticket ID is only in title, so not clickable. Manually opened ticket finally describes the context.

This is not very accessible. A PITA to handle.

As a user I would like to open the release, and see what is contained. At most I would expect to open the milestone, and be able to read through the enclosed ticket and PR titles and get an understanding of what changed (top level context).

[jzabroski]

I agree. I normally take the time but I am trying to work hard to pay the bills and do my day job. I realized I didn't do a release notes for this a month ago so I at least added a release with link to the milestones. I realized it's not great but at least its there.

[Kissaki]

I understand finding and committing time can be hard, especially for things one could consider additional polish. And descriptive metadata/structure doesn't necessarily come intuitive to everyone. I just wanted to point out how it ended up on my end, and that it'd be nice to have it. :)

Thank you for your efforts.