Html module improvement

[nojaf]

I already have the code for a PR but I'd like to have a discussion first whether people are on board with this. Here goes:

The Giraffe.XmlViewEngine module contains helper functions for constructing html. It does really feel right to have this as part of the xml engine. That is why I propose we moved the html specific bits to a separate module Giraffe.Html.

So I moved all the stuff and added helper functions for html attributes as well. See https://github.com/nojaf/Giraffe/blob/develop/src/Giraffe/Html.fs

I also re-exposed some stuff from the XmlViewEngine in this Html module so that you basically only need to add open Giraffe.Html to get everything.

type HtmlNode = XmlNode
let str = encodedText
let raw = rawText
let emptyText = XmlViewEngine.emptyText
let comment = XmlViewEngine.comment

The change from encodedText to str is to have more parity with Fable.React. The same reason that Attributes are in a separate module.

Some elements and attributes have the same name. I listed the workaround here.

Check out the Identity sample to see before and after.

Please let me know what you think, I'm looking forward to send the PR 😉

[dustinmoris]

Hi Florian,

thanks for the suggestion, I can see the reason for moving the HTML stuff into a separate module, but with the current suggestion I see the following problems:

• Name inconsistency: It would be Giraffe.XmlViewEngine vs Giraffe.Html. Maybe Giraffe.Xml and Giraffe.Html would be more consistent or Giraffe.HtmlViewEngine. Giraffe.Xml could be easily confused with generic XML serialization functions though, so probably wouldn't be a great name (and why I named it ...ViewEngine)
• Code duplication: I would like to avoid having to declare certain things twice only because of a name change, especially when the underlying code is pretty much the same. I think it makes maintenance a lot easier and also less likely for a contributor to forget a change or feature which would apply to both - XML and HTML.
• More modules which do very similar things. Overall the XML and HTML stuff would do very similar things and I am not sure if the difference between XML and HTML is big enough to justify a split. In general I tend to keep things together for simplicity and only split further out when there is a clear benefit or a very clear distinction.
• The current model is also more inline with the fact that a user needs to import only one module per view engine, whether it is Razor, DotLiquid, XmlViewEngine or something else in the future. I really like that about Giraffe... I think it reduces a lot of initial learning/"searching the docs" just to get a job done.

Overall I am not convinced that a split would add any real benefit. The current XmlViewEngine is not even that big yet, there's only very few actual functions and the rest is mainly tag declarations. I wouldn't even mind to add more to this file at the moment, like more tags (if there's any missing) and also more helper functions for attributes. I see these declarations as a static list - where rarely items would get modified, and the only real change would be more additions, which I find more maintainer friendly. It is easier to have the entire list in one file where one can search for existing tags/attributes and only add them when it's really missing.

This is obviously only my own two cents, but I wanted to give some reasons why I think I'd prefer to keep it in one file for now. Personally I try to look at these things very objectively and think about how is it actually maintainer and user friendly, rather than aiming for some optimisation, so if there's some clear benefits for maintainers and/or users then I'd be open to reconsider if you know what I mean!

EDIT: Maybe there would be also less of a desire for a split if the name XmlViewEngine would be different so that it feels more inclusive of the HTML functions too, because they are essentially very similar things. I'd be open for a module rename if that makes things better.

[nojaf]

Hello Dustin, thanks for taking the time to review my proposal. You have some strong arguments and I agree that splitting is in truth not the way forward.

I'm guessing I indeed had some problems with the XmlViewEngine name while it contains so much html helper functions and I personally only use it to construct html.

Moving forward I do think the attribute helper functions could still be useful. If they were to be added to the existing file, how would you deal with the clashing names? F.ex style is as well an element as an attribute.

[dustinmoris]

Moving forward I do think the attribute helper functions could still be useful. If they were to be added to the existing file, how would you deal with the clashing names? F.ex style is as well an element as an attribute.

Good point. Well that would be a good reason to move it into a separate module/namespace :)

[nojaf]

Hmm naming is still a bit tricky here. Are you thinking more towards a sub module Giraffe.XmlViewEngine.Html or go for a new module Giraffe.Helpers.Html?

I would be in favor to place the module in a separate file as well.

[dustinmoris]

Hmmm... what about Giraffe.XmlViewEngine.Attributes then (can be either a sub module or a separate file)?

[dustinmoris]

FSharpEngine, FunkyEngine, GiraffeEngine, GiraffeViewEngine, FuncEngine, FunEngine, Fungine, GEngine,...

If we'd rename Giraffe.XmlViewEngine, which one do you think would sound the nicest? Basically just need a more generic name as we agreed so that it is not closely associated with either XML or HTML.

[nojaf]

Naming is hard, I'm currently working on a project where I use this ViewEngine inside a Suave application (Long story short couldn't use Giraffe in combination with Mono, gave up and used Suave instead) so maybe it should have an unrelated name.

See handlebars, pug, Blade, Razor

So perhaps call it Elephant (just a random word). Or something else. I don't know to be honest.

[gerardtoconnor]

FSharpEngine, FunkyEngine, GiraffeEngine, GiraffeViewEngine, FuncEngine, FunEngine, Fungine, GEngine,...

We are going down a dark path ... 🤣.

If you want to make it a new GiraffeViewEngine I can re-write it to work the same but be way faster ... it is currently an allocation nightmare but no-one has complained so left it alone. For easy perf improvement without breaking api (precompiling parts to compressed byte []), we can simply change all tags to inline byte flags and have runtime renderer(er) write literals to body stream directly, templates remain the same. If no-one cares sure I'll leave it.

[forki]

don't call it Zebra!

@nojaf why would you want to use mono? Isn't everyone happy that we got rid of it?

[gerardtoconnor]

@forki too many things being called Zebra already :stuck_out_tongue_closed_eyes:

+1 on why use mono !?

[dustinmoris]

lol, ok.. actually... on a serious note... why not... just for fun, actually... call it... ZebraEngine :)

[dustinmoris]

If you want to make it a new GiraffeViewEngine I can re-write it to work the same but be way faster ... it is currently an allocation nightmare but no-one has complained so left it alone. For easy perf improvement without breaking api (precompiling parts to compressed byte []), we can simply change all tags to inline byte flags and have runtime renderer(er) write literals to body stream directly, templates remain the same. If no-one cares sure I'll leave it.

Ehm... that sounds amazing :). Sounds like that usage remains the same, so it would be great improvement!

[gerardtoconnor]

I may be able to borrow from Fable Compiler/AST and have function decomposed and compiled into a render state machine which would be fastest, but my proposed changes above are simple enough so will submit that for now.

[nojaf]

I'm using TypeProviders and I guess after reading this I still didn't know fully grasp if it is possible to run this on dotnet core. Feel free to ping me on the slack to have a talk about this.

+1 For perf, had no idea on that.

On a less serious note, I vote PelicanEngine :smile:

[forki]

TPs are possible in general. SQLProvider already works.

[gerardtoconnor]

@nojaf Typeproviders mostly working on .netcore now, Don has been hammering out the finer details and repos need to be updated but many working now.

[gerardtoconnor]

could it be done to re-use Fable React?

Could what be done? Fable React and this View engine are two very different animals for different purposes, This view engine is for basic server-side rendering with minimal dependencies and set up, React Fable is for a full flegged SPA client and requires alot more setup. @forki has been building a seperate GAFE tempate that uses Fable React with Giraffe as render engine. (almost done I believe ?)

[dustinmoris]

@gerardtoconnor Before you start any refactoring, would you be happy naming it GiraffeViewEngine ?

[forki]

Yes. https://github.com/SAFE-Stack/SAFE-BookStore/pull/255 is done. Just waiting for Giraffe to do something about negotiate issue.

Am 20.12.2017 15:17 schrieb "Gerard" notifications@github.com:

could it be done to re-use Fable React?

Could what be done? Fable React and this View engine are two very different animals for different purposes, This view engine is for basic server-side rendering with minimal dependencies and set up, React Fable is for a full flegged SPA client and requires alot more setup. @forki https://github.com/forki has been building a seperate GAFE tempate that uses Fable React with Giraffe as render engine. (almost done I believe ?)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/giraffe-fsharp/Giraffe/issues/138#issuecomment-353074328, or mute the thread https://github.com/notifications/unsubscribe-auth/AADgNH4Txco0lLLWz-PCPkyDJoJ4DPCbks5tCRbpgaJpZM4Qaj_5 .

[nojaf]

@gerardtoconnor nevermind that, the idea was to re-use the functions Fable.React uses. But they are lead to native js.

[gerardtoconnor]

@dustinmoris I am pretty agnostic and indifferent on the naming so others can chime in as they wish ... GiraffeViewEngine seems like a clean simple option.

@nojaf we can go and extend with any functions we want from Fable.React provided they are non-breaking and no client/js dependencies.

[dustinmoris]

Thanks for the work on this, closing now.

[gerardtoconnor]

Just as quick follow up on Html Rendering engine.

https://github.com/gerardtoconnor/ViewEngineTests

the long and short of it was, although I could get render to be 5.6x faster (is using in-memory stream), using IO, to filesystem/sockets, diluted the perf gain down to negligible, like 6-7% better, not really worth it unless IO bottleneck improves somehow. I only did micro benchmarks so might be more material on server with concurrent rendering & bigger templates ... but still not the x2+ speed up I was hoping for.

Testing various alternative view engine implementations:

• XmlView - original Suave style template
• GiraffeView - struct/enum variation
• ByteView - lightweight streamwriter nodes
• TemplateView - Alt syntax templating engine of compiled components

1 - using memory stream to assess raw performance without IO

BenchmarkDotNet=v0.10.11, OS=Windows 10 Redstone 3 [1709, Fall Creators Update] (10.0.16299.192)
Processor=Intel Core i7-4770K CPU 3.50GHz (Haswell), ProcessorCount=8
Frequency=3415989 Hz, Resolution=292.7410 ns, Timer=TSC
.NET Core SDK=2.1.4
  [Host]     : .NET Core 2.0.5 (Framework 4.6.26020.03), 64bit RyuJIT DEBUG
  DefaultJob : .NET Core 2.0.5 (Framework 4.6.26020.03), 64bit RyuJIT

Method	Mean	Error	StdDev	Scaled	ScaledSD	Gen 0	Allocated
GiraffeView	4.739 us	0.0504 us	0.0447 us	3.11	0.03	1.4801	6240 B
XmlView	8.586 us	0.1155 us	0.1081 us	5.63	0.07	3.3417	14040 B
ByteView	1.525 us	0.0053 us	0.0044 us	1.00	0.00	0.2899	1224 B
ByteViewAsync	5.287 us	0.0427 us	0.0400 us	3.47	0.03	1.1673	4904 B
TemplateView	1.407 us	0.0122 us	0.0114 us	0.92	0.01	0.1469	624 B

2 - Test on filesystem to see how much IO impacts perf and if benfit material

BenchmarkDotNet=v0.10.11, OS=Windows 10 Redstone 3 [1709, Fall Creators Update] (10.0.16299.192)
Processor=Intel Core i7-4770K CPU 3.50GHz (Haswell), ProcessorCount=8
Frequency=3415989 Hz, Resolution=292.7410 ns, Timer=TSC
.NET Core SDK=2.1.4
  [Host]     : .NET Core 2.0.5 (Framework 4.6.26020.03), 64bit RyuJIT DEBUG
  DefaultJob : .NET Core 2.0.5 (Framework 4.6.26020.03), 64bit RyuJIT

Method	Mean	Error	StdDev	Scaled	ScaledSD	Gen 0	Allocated
GiraffeView	240.4 us	4.754 us	6.664 us	1.00	0.04	2.4414	10.3 KB
XmlView	256.0 us	3.271 us	3.060 us	1.07	0.03	3.9063	17.91 KB
ByteView	239.5 us	4.627 us	6.017 us	1.00	0.00	1.2207	5.39 KB
ByteViewAsync	337.9 us	5.047 us	4.214 us	1.41	0.04	4.8828	2.69 KB
TemplateView	244.2 us	4.527 us	5.031 us	1.02	0.03	0.9766	4.81 KB

[dustinmoris]

@gerardtoconnor Nice, thanks for those tests! I think I'll have to speak to you in Slack to fully understand each of these options!