-
Notifications
You must be signed in to change notification settings - Fork 5
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
Added HTML encoding to FsHtml library and mechanism for including raw… #6
base: master
Are you sure you want to change the base?
Conversation
|
||
//dump (tree1, 9) | ||
Results.Print 6 tree1 |
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.
Oh, does that work now? I think it would break with stack overflow exception last time I checked?
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.
I'm not sure it broke before. I think the problem was the default depth of dump. The example tree is an infinite binary tree and dump had a default depth of 100. My experience with object dumpers is you have to very conservative with the default depth because it can get out of hand very quickly. Even a default depth of 10 can easily run out of memory in real work conditions.
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.
@scrwtp maybe you are confused with stackoverflows coming by the typeshape part.
I think if a shape is not handled by Typeshape it falls in the default case which Stackoverflows if the shape has loops.
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.
Ah, fair enough, maybe it was just the depth then. Or I was remembering it from a time where the depth limit was not being honored.
@gusty yeah, my point was that I commented it out for a reason, so I was surprised that changing in a review that didn't touch the traversal part (in the "cool, why does it work now" sense ;))
FsHtml was lifted from https://github.com/ptrelford/FsHtml and I was planning to remove the file from FsPad and reference it through Paket. So in general that would be a more suitable place to send this PR to. I think the idea is good, but the implementation of these changes doesn't go in the right direction. I'm not the author, but I think FsHtml treats encoding as a separate concern and leaves that to the client on purpose, as to not force an opinion whether the strings used should or shouldn't be already encoded. And What I could see working (and perhaps merged back to the original FsHtml repo) would be including a helper function or static member on |
Phil hasn't put a lot of time or attention into this library. There's a number of basic pull requests that have been sitting around for a few years that fix obvious bugs. ( ptrelford/FsHtml@4bfb3d6 ) There's not a lot of love in this library. I can't speak for Phil, but one thing I've noticed about him is he will purposefully elide details like HTML encoding and performance if it distracts from teaching a bigger idea. This particular library strikes me that way and I'm sure it would be a lot better if he put his heart into it. HTML libraries used in production tend not to ignore HTML encoding and don't use simple string concatenation (especially in .NET) to build the final HTML. Don't get me wrong. I love it. It's a wonderfully simple library/DSL with a clean surface area, but it does needs some basic tweaking on the implementation. Have you checked out Elm or the Fable/Elmish libraries? If not, I highly recommend it. The Elm guys have done an stunningly amazing job at creating a language / development environment / DSL that is fast & reliable environment for building rock solid, interactive front-ends. What's nice is one can often copy + paste Elm templates into F# and do very little tweaking to get them working. |
@scrwtp Can we hit this from another angle? What are your thoughts of leveraging the IHtmlString interface in the dumper? https://msdn.microsoft.com/en-us/library/system.web.ihtmlstring(v=vs.110).aspx There are a lot of ways you could leverage it. What are your thoughts? |
@sgoguen Sorry but I'm a bit lost. What is the problem with the current approach? |
I really don't want to harp on adding HTML encoding to FsHtml if you're not into it, because ultimately it's only being used internally by the dumper. If it was being used as a general DSL for other people to use, I'd make a much stronger case for HTML encoding by default. It's not very consequential in this case. So, I figured I'd move onto a tangentially related idea and ask you guys what your thought was on dumping objects that are capable of rendering themselves in HTML. LINQPad provides a feature where you can create custom dumpers for object types by simply implementing a ToHtml method. I've used (and implemented this feature in other things), to create generic containers. For example, I have a Razor helpers along the lines of: type containers<'a> =
| Tabs of record:'a // Uses property names for tabs
| Page of record:'a // Uses property names for headers
| DefinitionList of record:'a // You get the idea... As you can see, I use these types as a way to hint how I want something rendered in a way that's orthogonal to my object dumper. I just tag the type somehow (Implement an interface, define and tag a function somewhere) so my dumper knows to call a different function and pass it through. I wanted to get your thoughts before I did something. |
@sgoguen, to give you some background, the decision to use FsHtml over another static html solution was dictated purely by what's the simplest, least likely to backfire thing to put in place in a short manner of time. We are not in any way invested in FsHtml here. We did in fact briefly discuss ditching static html and using some JS templating library on the browser, but we felt this would have been an extra effort orthogonal to what our goals were at the time. The main focus of the work we were doing was to separate traversal of the inspected value (Typeshape-based printer) and html generation, and we got there by introducing this intermediate set of types defined in Representation.fs. This intermediate representation can be consumed in any number of ways, including but not limited to our current static html approach. For instance, you could feasibly provide an implementation of a LinqPad viewer through the means you mentioned. My understanding of how FsPad started however was that @gusty intended to detach this tool from LinqPad. You have stated yourself that you want to have FsPad running on non-windows platforms, so I'm not entirely sure why is LinqPad part of the conversation again? What happened with the plan of using Suave? |
@scrwtp I was looking at your commit last night and I realized how irrelevant FsHtml was when I started digging into it, and I did notice you are working toward a representation that can be marshalled to a browser client. ( This is really nice progress BTW ) Seeing that, I pulled it into my fork that uses Suave and websockets and it worked really well. I'm stoked! |
@scrwtp I added some default HTML encoding in the FsHtml library itself and included an extension point to the Html union type to accommodate inserting the raw HTML (For the the static header)
Thoughts?