search

toddams / RazorLight

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

Fixes #287: Make TemplateRenderer stateless #310

Closed matthewwren closed 4 years ago

matthewwren commented 4 years ago

This was my suggestion to make the TemplateRenderer stateless to avoid the issues that caused the failure of Layout to be rendered when using await IncludeAsync: https://github.com/toddams/RazorLight/issues/287

jzabroski commented 4 years ago

@toddams What's your thoughts on this change? It makes sense but breaks public API. But Matt makes a good case that the current public API isn't optimal.

toddams commented 4 years ago

I agree, there is an enormous room for improvements in the current project. We need more tests, though, in current state it covers very little piece of a critical functionality

jzabroski commented 4 years ago

Hmm.. what tests do you have in mind? If you describe them, maybe I'll code them up.

toddams commented 4 years ago

l don't mean this particular Pull Request, sorry for the confusion. We apply this MR and we don't know if it will break something, simple as that )

srace4050 commented 4 years ago

@toddams any idea when this PR will get merged? A lot of users are waiting for the new version to address this issue. CC: @jzabroski thank you for the PR.

toddams commented 4 years ago

Let's fix conflicts and merge it

srace4050 commented 4 years ago

@jzabroski can you please fix the conflicts?

hellokids86 commented 4 years ago

waiting for this too.

srace4050 commented 4 years ago

yes, waiting for this PR to get merged.

chancie86 commented 4 years ago

Also waiting on this - just diagnosed and found the issue myself, then discovered this PR. Doh!

If there's anything I can do to speed up getting this merged in, please let me know!

chancie86 commented 4 years ago

As there's been no activity on this PR for 2 months, I've created a new one with the conflicts resolved.

srace4050 commented 4 years ago

@chancie86 Thank you! @toddams can you please look into it.

jzabroski commented 4 years ago

@srace4050 @chancie86 @toddams Merged and pushed 2.0.0-beta5 package to nuget with symbols. On GitHub Releases, I messed up the tags ever so slightly - I forgot to bump the Version metadata prior to doing the release - does anyone know if its OK to delete the tag and replace it with a new one? Will GitHub Releases re-generate the assets, etc?

jzabroski commented 4 years ago

@matthewwren , @chancie86 was nice enough to preserve you as author on his PR, so you still get credit in the log as true author. -- @chancie86 , any chance you can tell me how you did that?

matthewwren commented 4 years ago

Good collaborating with you guys on this one, thanks @chancie86 and @jzabroski

chancie86 commented 4 years ago

@matthewwren , @chancie86 was nice enough to preserve you as author on his PR, so you still get credit in the log as true author. -- @chancie86 , any chance you can tell me how you did that?

Didn't want to take all the blame, haha :D . I branched off master, made a PR from Matt's branch into mine. Then followed instructions on github for resolving the conflict. Once that was done it was just a case of building the tests, running the unit tests and then the usual push/PR. I'm pretty new to git tbh - it feels more complicated than it needs to be.

Thanks for the quick release!