Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sgoguen
Copy link
Contributor

@sgoguen sgoguen commented Mar 19, 2018

@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?


//dump (tree1, 9)
Results.Print 6 tree1
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Owner

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.

Copy link
Collaborator

@scrwtp scrwtp Mar 19, 2018

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 ;))

@scrwtp
Copy link
Collaborator

scrwtp commented Mar 19, 2018

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 RawHtml is only necessary when you start encoding text in FsHtml. Otherwise it's not different to what you can already express with Text case.

What I could see working (and perhaps merged back to the original FsHtml repo) would be including a helper function or static member on Html type for encoding text that would do the encoding and yield the result as a Text.

@sgoguen
Copy link
Contributor Author

sgoguen commented Mar 19, 2018

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.

@sgoguen
Copy link
Contributor Author

sgoguen commented Mar 20, 2018

@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?

@gusty
Copy link
Owner

gusty commented Mar 20, 2018

@sgoguen Sorry but I'm a bit lost.

What is the problem with the current approach?

@sgoguen
Copy link
Contributor Author

sgoguen commented Mar 20, 2018

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.

@scrwtp
Copy link
Collaborator

scrwtp commented Mar 22, 2018

@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?

@sgoguen
Copy link
Contributor Author

sgoguen commented Mar 22, 2018

@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants