-
Notifications
You must be signed in to change notification settings - Fork 261
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
Implement the RazorLight.Precompile tool #492
Conversation
This Pull Request provides an answer to my question raised in the issue #491 |
b02afbc
to
30a7cd4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, but this isn't happening, at least not in the format you proposed. It is especially bad that you're just swallowing exceptions - that is a non-starter. The interface complexity of the callback is also very high - I don't think anyone would know what this does without reading the source code.
There is a Precompile project for compiling views, but it hasn't been finished. Would you be interested in building a .NET Global Tool that can do that?
{ | ||
callback(razorTemplate, rawAssembly, rawSymbolStore); | ||
} | ||
catch (Exception) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Swallowing exceptions is bad. How will the end user know if there is any problems with the callback?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my defense I followed the exact same scheme as used for the already existing prerenderCallbacks
-
RazorLight/src/RazorLight/TemplateRenderer.cs
Lines 223 to 239 in 22381db
private void ExecutePageCallbacks(ITemplatePage page, IList<Action<ITemplatePage>> callbacks) | |
{ | |
if (callbacks?.Count > 0) | |
{ | |
foreach (var callback in callbacks) | |
{ | |
try | |
{ | |
callback(page); | |
} | |
catch (Exception) | |
{ | |
//Ignore | |
} | |
} | |
} | |
} |
I figured if that is good enough for them, why should it be different for these? However, I am will gladly change it, although I do not see any regression as opposed to the prerender callbacks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a fair point, but very few people actually utilize those callbacks because they're not easily discoverable. I've found most people follow a really old tutorial from medium.com for how to use RazorLight that is woefully out of date and contains some rather questionable decisions, that resulted in me spending way over a month figuring out why people were reporting bugs just "standing up RazorLight" that I couldn't fathom happening until I found that online tutorial's faulty guide.
|
||
public RoslynCompilationService(IMetadataReferenceManager referenceManager, Assembly operatingAssembly) | ||
public RoslynCompilationService(IMetadataReferenceManager referenceManager, Assembly operatingAssembly, List<Action<IGeneratedRazorTemplate, byte[], byte[]>> precreateAssemblyCallbacks = null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The interface here is really not "discoverable" - I have worked on this code base for over 4 years now, and just looking at this callback makes my brain hurt. The lack of a default implementation also further increases the odds people will use it incorrectly or copy-paste some bad code from somewhere on the Internet, and we will get a lot of support Issues dealing with lazy programming. - This is the top maintenance cost of this project thus far.
- Why are you calling it precreate vs. precompile?
- If your goal is to precompile assemblies, why not contribute a PR to the RazorLight.Precompile csproj?
- I think you're trying to do too many things in one pass - using a separate tool to precompile the views makes each tool quadratically less difficult to test, as you are testing A + B services area rather than the square of the number of states in A and B.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My reply would be this:
- How is it different from the prerender callbacks?
- My rationale is that it is called before the Assembly is created. The code has already been compiled, so it does not happen before the compilation. But if
precompile
is more recognizable, then it makes sense to rename. I guess this has to do with discoverability.precompile
is a known buzz word,precreate
is not. So, makes sense to rename. Unless, we want to keep it obscured :-). - I do not know. I was not aware of this tool. I did notice the project, but since it was not mentioned in the documentation (or I missed it) I just ignored it. What is the state of this tool?
- Makes sense in general.
TBH, I am acting on my selfish reasons - I need to implement something in Powershell yesterday and as it is either I drop RazorLight entirely (in favour of what exactly? as far as I know there is nothing being as popular and actively maintained) or complicate a straightforward and conceptually simple scripting code to move all the rendering to one executable call.
If RazorLight.Precompile
is already usable, then definitely I would prefer using it. But it may be a longer engagement and in the meantime - the ability to cache generated assemblies is needed now. At least I need it now.
Can we find a compromise that would allow us to release something quickly yet satisfying the quality requirements? I will gladly implement all your recommendations, if at the end I will be able to achieve my goal in a timely fashion.
Hope it makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If
RazorLight.Precompile
is already usable, then definitely I would prefer using it. But it may be a longer engagement and in the meantime - the ability to cache generated assemblies is needed now. At least I need it now.
I'm not trying to create work for you, but it seems straight forward to lift your PR into a tool vs. something that caches everything on startup. The RazorLight.Precompile project is just a stub right now.
if at the end I will be able to achieve my goal in a timely fashion.
I'm not trying to create work, but I do think most people would rather precompile this stuff vs. create a persistent cache. It makes the entire flow a lot more debuggable since you have to use the compiled DLLs on disk vs. use a persistent cache that could get turned off via misconfiguration or load a different projects cached items by accident.
I have changed the PR to actually implement the RazorLight.Precompile tool. |
I think it makes sense to abandon this PR and replace it with a new one. |
I have just added an ability to render the given model by the precompile command. Please, run |
Hi Mark. I hope you are well. I have an immediate family member in the hospital so this has been on hold for now. I promise it's not simply being ignored |
@jzabroski - sorry to hear that. Hope things get better. Thanks for letting me know. |
Getting to this now. Expect a reply in the next 3 days. I do think it looks good though |
Thanks, Mark. Thank you as well for your patience while I worked through some things personally. |
No problem. Thank you for merging. Does it going to publish to nuget.org automatically? |
No, I have to do that but I will do it now assuming I can remember how I set it up on GitHub & the API key is not expired. :-) |
@MarkKharitonov It looks like there is one failing test after I merged. Can you check? I am not immediately clear why it happened but I think its due to the backslash separator is not compatible with Ubuntu Linux? Path separator are platform dependent : For windows, it’s \ and for unix it’s /.
|
I will have a look |
I think you just need a platform check. We use xunit, which has some platform specific features built-in via plugins. But if the test is only for .NET 6, you can also just use the .NET 6 platform abstraction APIs to detect the platform elegantly. |
Actually you can probably just use https://learn.microsoft.com/en-us/dotnet/api/system.io.path.pathseparator?view=netframework-4.8 since it works across all versions |
I need to setup a local linux environment to test, because it all passes on Windows. |
I honestly dont think you need to go that far :-) You can fork GitHub repo and set-up actions locally in your repo using our template and just make the one line change i suggested to fix the issue. Have you ever just hit |
I think there should be a PR workflow on this repository to prevent future inconveniences. Anyway, I am able to run the workflow from my branch in my fork and I can see the problem. Will fix it. |
This method allows to implement a persistent cache as opposed to the existing in-memory cache.
I do not include an implementation of such a cache. But an example of how this can be done is:
And then it can be used like this: