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

IncludeAsync should not default to ".cshtml" #359

Open Tyrrrz opened 4 years ago

Tyrrrz commented 4 years ago

Describe the bug

When you use IncludeAsync along with embed resource provider, it will always append .cshtml to the end of the key. I'm trying to include a CSS file to embed it inside the page, but I can't, unless it ends in .cshtml.

To Reproduce Steps to reproduce the behavior:

@{ await IncludeAsync("Core.css"); } // tries to include "Core.css.cshtml"

Expected behavior

Should properly include Core.css instead of trying to resolve Core.css.html.

Information (please complete the following information):

Additional context Add any other context about the problem here.

jzabroski commented 4 years ago

What is the correct behavior? Whitelist various extensions other than the alternative extension?

The issue is DefaultExtension = ".cshtml"

Tyrrrz commented 4 years ago

I would suggest that if an extension is already provided then it shouldn't be appended. Only append if it's not, i.e. Core instead of Core.css or Core.cshtml.

jzabroski commented 4 years ago

The problem I see with that proposal is that folder slashes get converted to ".", so how would you know if something is a subfolder to a cshtml vs. a css file? I realize the odds someone names their file Views/css.cshtml are small, but how would you suggest we deal with that resolving to Views.css?

var assembly = Assembly.GetExecutingAssembly();
var resourceName = "Views.css.cshtml";

using (Stream stream = assembly.GetManifestResourceStream(resourceName))
using (StreamReader reader = new StreamReader(stream))
{
    string result = reader.ReadToEnd();
}
Tyrrrz commented 4 years ago

What about, try to open resource Views.css, if it exists then go with that, otherwise try Views.css.cshtml?

jzabroski commented 3 years ago

@Tyrrrz , as an update here, @maxbanas has improved the logic around error messages / thrown exceptions when a template cannot be found. So, I am not as opposed as I used to be towards changing this behavior.

Thinking about this and the optimal design, ideally, if we had support for tag-helpers, wouldn't you just use a TagHelper to IncludeAsync your css off a <style /> tag via StyleTagHelper extension?

To be honest, given the amount of work to ship TagHelpers, I am not opposed to something smaller, like, maybe an IncludeFileContentsAsync, that functions like you describe? You have good aesthetic taste, so feel free to say adamantly what you think is right here.

Tyrrrz commented 3 years ago

@jzabroski thanks for getting back about this!

I actually ended up realizing that I was only using about 10% of RazorLight's features and have a few rather niche requirements (like being able to use internal models in templates), so I created a very simple alternative for myself. I'm currently using C# source generators to transpile Razor templates into C# classes at build time.

Back to the question, I think IncludeFileContentsAsync seems more straightforward to me. Essentially, I'm looking for an alternative to IncludeAsync that just imports the contents of the file as is without treating it as a template, which IncludeFileContentsAsync appears to convey well in its naming.

I'm not sure exactly how StyleTagHelper works, so maybe it would also solve the problem. For my use case, I needed to choose one of two CSS files based on the selected theme (a property in the model). The CSS files are packed the same way the templates are, which is EmbeddedResource, so it would be nice if there was a way to tap into RazorLight's project system to resolve the file.

You have good aesthetic taste, so feel free to say adamantly what you think is right here.

I was surprised to read that, thank you 😊 Hopefully my answer helped a little.

jzabroski commented 3 years ago

I'm not sure exactly how StyleTagHelper works, so maybe it would also solve the problem.

On-Topic: You'd just define a StyleTagHelper with a custom razorlight-src helper attribute that would let you write:

<style razorlight-src="View.css" />

and it would internally call the equivalent of IncludeFileContentsAsync effectively.

Off-topic: I have various ways I've loaded CSS into web pages over the years. By far my favorite conceptually was when I was using RequireJS AMD with the requirejs/text plugin, as it enabled me to build TypeScript components that fully packaged a ViewModel, html template and css in a modular way that avoided including the same CSS multiple times. It also was nice in that it made it clear that a component had some assumptions on how it was styled, so the html piece and css piece had clear contracts with the overall component. This had the added benefit I could create an arbitrary MVC endpoint that would just... load a TypeScript component given some jsonConfig. I personally think moving away from such paradigms is a mistake, but as I get older, I'm less rebellious and more compliant with what everyone else is doing.

@jzabroski thanks for getting back about this!

No problem. I just wish I had more free time to just code for fun and work on these projects. While I use these projects professionally, I never knew people had so many diverse use cases.

Tyrrrz commented 3 years ago

On-Topic: You'd just define a StyleTagHelper with a custom razorlight-src helper attribute that would let you write:

Yeah, that would've been good enough for me. I can't immediately think of other use cases right now, but maybe IncludeFileContentsAsync would still be useful when you need to include something that is neither cshtml nor css.

No problem. I just wish I had more free time to just code for fun and work on these projects. While I use these projects professionally, I never knew people had so many diverse use cases.

No worries, your efforts are always appreciated!

jzabroski commented 3 years ago

I think the simplest thing here is to construct a FileSystemRazorProject where DefaultExtension = string.Empty. Similarly, the DI helpers can have a WithOption helper.

jzabroski commented 3 years ago

FileSystemRazorProject.Extension was marked as virtual, and had a DefaultExtension value, which could be configured via constructor.

EmbeddedResourceRazorProject.Extension was not marked as virtual, and had an explicit default value of ".cshtml". I suppose you could override this by sub-classing EmbeddedResourceRazorProject and defining a different default in that constructor, but that is very ugly.

End-game is to true these up. Both project types should support constructors where you pass in the default extension, and tests should cover that it is ok to pass in an string.Empty value for default extension. In that case, View.css should work (I hope)