toddams / RazorLight

Template engine based on Microsoft's Razor parsing engine for .NET Core
Apache License 2.0
1.51k stars 261 forks source link

Default Namespaces does not appear to work #387

Closed rburnham52 closed 3 years ago

rburnham52 commented 3 years ago

Describe the bug When using the following setup. Adding Default Namespaces appears to have no effect as i am receiving messages similar to

Failed to compile generated Razor template:

  • (3:17) The name 'Guid' does not exist in the current context

See CompilationErrors for detailed information

To Reproduce I've broken it down to the simplest console app i could make.

    class Program
    {
        static async Task Main(string[] args)
        {
            var defaultNamespaces = new[]
            {
                "System",
                "System.Linq",
                "System.Collections.Generic",
                "RazorLight.Text",
                "RazorLight"
            };
            var engine = new RazorLightEngineBuilder()
                .UseEmbeddedResourcesProject(typeof(Program))
                .AddDefaultNamespaces(defaultNamespaces)
                .UseMemoryCachingProvider()
                .Build();
            var model = new ViewModel() {Name = "test"};
            var result = await engine.CompileRenderStringAsync("test.cshtml", 
                 @"
                    @model ConsoleApp1.ViewModel
                    @{
                        Model.Name = Guid.NewGuid().ToString();
                    }
                    <p>@Model.Name</p>
                ",
                 model);

            Console.WriteLine(result);
            Console.ReadLine();
        }
    }

Expected behavior It should be able to find types in the default namespaces added during setup.

Information (please complete the following information):

Additional context This was the exact usage that FluentEmail was using, I was trying to setup FluentEmail to support default namespaces and could not get it to work.

maxbanas commented 3 years ago

It looks like default namespaces are ignored for TextSourceRazorProjectItem's. Your example seems to work when I comment out those lines, but based on this test, this behavior appears to be intentional.

rburnham52 commented 3 years ago

This seems to be the method FluentEmail.Razor uses, I assume because it makes sense to embed your email templates. Is there an alternative that would work with embedded templates?

jzabroski commented 3 years ago

I would have to dive into the twists and turns in the cave to say for sure, but I think the reason this test exists is because string rendering doesn't have a project system, and I think (to date) the code assumes you use a project system to opt in to (a) tag helpers (b) default namespaces. Exactly why this assumption needs to be true, I don't really know and/or have a good reason other than it complicates the interface for actually rendering templates. As an example, an alternative project system (to bypassing the project system and just doing string rendering) would be one that lets you access templates from main memory by storing them in some dictionary.

maxbanas commented 3 years ago

@ryburn52 I'm not sure if I'm understanding correctly, but if you are embedding your template then default namespaces should work. For example, if you have an embedded resource file test.csthml, with the content:

@model ConsoleApp1.ViewModel
@{
  Model.Name = Guid.NewGuid().ToString();
}
<p>@Model.Name</p>

You would use engine.CompileRenderAsync("test", model); and your default namespaces should be picked up. (instead of CompileRenderStringAsync)

As a side note, it looks like System is already added automatically.

jzabroski commented 3 years ago

Yes, this should work for embedded templates, but it cannot work for text templates. Somewhat related is the discussion @maxbanas and I are having in the other threat, where RazorSourceGenerator doesnt know the project system a project item comes from. As a result, there is no clean way to pass down default namespaces.

The simplest workaround to try would be to prefix your test templates (raw text templates) with the default namespaces. Can you please try that? (But agree with @maxbanas that it sounds like you are using raw strings and NOT embedded templates.)

jzabroski commented 3 years ago

@ryburn52 Looking at your program more closely, I need to confirm a hypothesis, but I believe we added an overload for raw string rendering and it looks like you are calling raw string rendering while using an embedded template. If so, that is probably why I was not psyched about supporting a raw string rendering overload (but I did it because a lot of people were using raw string rendering due to popular online tutorials that bypass RazorLightEngine). It was probably a mistake - I will have to dive in further to investigate but I thought a fast reply now might save you hours of time if I'm right.

rburnham52 commented 3 years ago

@jzabroski @maxbanas Thanks for the update, So this is the Method FluentEmail.Razor uses

https://github.com/lukencode/FluentEmail/blob/4b9740bb1b1df9750f0d7043f6ca6c72bc39b86f/src/Renderers/FluentEmail.Razor/RazorRenderer.cs#L40

Not my project but i was looking at doing a .net core upgade and adding support for default namespaces.

So it looks like the issue is that they are trying to support multiple template methods,

  1. String
  2. Razor File
  3. Embedded Razor file.

This all makes sense, They have a constructor overload for each one. However when it comes to rendering they are falling back to the most common denominator, by using the string template. I can see how this would make sense normally, You wouldn't expect there to be a behaviour difference between the 2.

Update In writing this comment i noticed that the master version of this file had already been updated to use CompileRenderAsync The branch i cloned is using CompileRenderStringAsync so i'll need to check to see what happened.

As a side not it still seems strange to me that you wouldn't want to support default namespaces when using a string template. Could the string method just use a dummy project that sets up the minimal needed, eg default .net namespaces like System, System.Collections.Generic?

rburnham52 commented 3 years ago

Based on this discussion though, i would expect to see both compile methods used depending on the project?

It looks like the CompileRenderAsync signature has changes in the latest version, you no longer provide a template. How does it know what template to use? is this matched based on the key param depending on the project type? eg if it's file system then a path to the file, if embedded then its the fully qualified name?

rburnham52 commented 3 years ago

Yeah it looks like this was changed to the CompileRenderStringAsync overload in the PR i was branching off, https://github.com/lukencode/FluentEmail/pull/186/files#

Probably because the signature changed?

maxbanas commented 3 years ago

@ryburn52

It looks like the CompileRenderAsync signature has changes in the latest version, you no longer provide a template. How does it know what template to use? is this matched based on the key param depending on the project type? eg if it's file system then a path to the file, if embedded then its the fully qualified name?

Pretty much, I think. The key is provided to the RazorLightProject project through the Task<RazorLightProjectItem> GetItemAsync(string templateKey) method. The exact key path depends on the options you pass to the .UseEmbeddedResourcesProject or .UseFileSystemProject builder methods.

As a side not it still seems strange to me that you wouldn't want to support default namespaces when using a string template. Could the string method just use a dummy project that sets up the minimal needed, eg default .net namespaces like System, System.Collections.Generic?

This is kind of strange to me too. Especially considering that some engine builder options like DisableEncoding() work with CompileRenderStringAsync, but this option doesn't. If the option was specific to a project, I would think it would be provided as an argument in the Use...Project method rather than as an engine builder option.

@jzabroski

Yes, this should work for embedded templates, but it cannot work for text templates. Somewhat related is the discussion @maxbanas and I are having in the other threat, where RazorSourceGenerator doesnt know the project system a project item comes from. As a result, there is no clean way to pass down default namespaces.

It looks like namespaces provided in RazorLightOptions via the AddDefaultNamespaces builder method are already passed directly (in the form of ISet<string>) to RazorSourceGenerator through its constructor, so unless I'm missing something, RazorSourceGenerator should already have these. But based on the tests, this behavior definitely appears to be purposeful, and like you, I'm not exactly sure why.

jzabroski commented 3 years ago

@maxbanas Thanks, that's great research. I doubt Ivan remembers why and I try not to bug him these days as he hasn't been involved in a while.

rburnham52 commented 3 years ago

@maxbanas @jzabroski Thanks for the help, Considering that this would either require a change to Razor Light or a Major refactor of Fluent Email so it relies on Razor Light Projects for embedded templates. We've decided to try a different email library.

We were using Postal so we'll try the Postal .net core fork