asvd / microlight

highlights code in any programming language
http://asvd.github.io/microlight
MIT License
1.48k stars 63 forks source link

Added method for direct text-to-HTML conversion #1

Open buu700 opened 8 years ago

buu700 commented 8 years ago

microlight.reset still exists for automatically traversing the DOM and performing in-place highlighting, but the core logic has been factored out as microlight.process for performing the same operation on a string input without side effects.

buu700 commented 8 years ago

Also, it just occurred to me, I probably should've included this to begin with; here's a diff with indentation removed to make it more clear what actually changed: microlight pr diff.pdf

blueimp commented 8 years ago

@buu700 You can display a diff without whitespace changes with the ?w=1 argument, e.g. for this pull request: https://github.com/asvd/microlight/pull/1/files?w=1

See also https://github.com/blog/967-github-secrets

P.S.: Really nice, minimal library @asvd . :)

asvd commented 8 years ago

@buu700 thanks for your suggestion.

I would not merge your changeset, since I don't see a real reason for highlighting a text virtually (yet I am curious to know your thoughts on how this can be used). And besides this would increase the library size (I'm trying to keep it as minimal as possible mainly for fun).

By the way: you don't need to create a DIV element which is only used for storing the HTML content. Just put it into a string and then return.

buu700 commented 8 years ago

Nice, thanks for the tip @blueimp! That feature should really be way more prominent.

As far as the use case @asvd, this is how I would be using it at least: https://github.com/cyph/cyph/blob/next/shared/js/cyph/ui/directives/markdown.ts#L31. I'm not sure how common that type of usage would be, but it seems like a standard enough pattern (and addresses the complaint someone on HN had about it directly modifying the DOM).

As far as the size, are you specifically referring to the increase from 2.2kb to 2.3kb, or just making the more general point that you'll want to be conservative in what you add? FWIW, the gzipped size remains at 1.4kb with or without this change.

Got it, thanks for the clarification on the createElement thing. (I hadn't actually looked closely enough at the code to determine whether that was necessary; was mostly trying to make as few changes as possible.) I'll update this PR with that fixed in a bit.

asvd commented 8 years ago

I'm just preparing a more extended version of microlight, which will support editable areas, and also include some performance tweaks. So I can consider your suggestion to be added there, but not into microlight.js which I would like to keep in a minimal state.

What happens then in your project with highlighted code? Where do you put it then? Do you want to perform the highlight on server side? Because in this case the formatted text can become bigger than the library itself, as discussed in this branch on HN: https://news.ycombinator.com/item?id=11803389

buu700 commented 8 years ago

Fair enough, I'll just maintain this version as a separate fork until the extended version is available.

Ah sorry, I should clarify more specifically what we're actually doing. @cyph currently includes highlight.js in production for allowing users to send snippets of code to each other, which can only be done client-side because the context is user-generated content within a secure/E2EE chat.

screen shot 2016-06-01 at 11 37 19 am

That currently looks pretty bad because we hadn't yet gotten around to setting up a good custom style, but Microlight out of the box turned out to be great (in addition to the reduced footprint):

screen shot 2016-05-31 at 12 50 42 am
asvd commented 8 years ago

You have an editable area there, so you will definitely need an extended version :-)

Why not send code not formatted, and highilght upon rendering? That would reduce the amount of data to exchange..

buu700 commented 8 years ago

Yep, it sounds like the extended version would probably be relevant there.

That is what we're doing; what made you think otherwise? The text sent over the wire is just Markdown (foo.bar()).

asvd commented 8 years ago

Then it should work with the original version of microlight. You get the non-highlighted code, put it into a div with class=microlight, invoke microlight.reset() and have it highlighted. Right?

buu700 commented 8 years ago

Yeah, more or less; I would just have to change that line I linked to something like highlight: s => <div class='microlight'>${s}</div> and call microlight.reset() after it hits the DOM, as you noted.

The problem is that it would then have to bypass our post-processing logic (which currently does nothing that would impact code highlighting, but could eventually) and DOMPurify sanitisation, which would be particularly problematic for us in the event that someone were to find an XSS exploit against Microlight (probably unlikely given Microlight's limited scope, but all it would take would be for one browser to interact weirdly with Microlight in some edge case).

buu700 commented 8 years ago

Just made the change you suggested earlier. Not that it really addresses your issue with accepting this PR, but the minified file size is back down to 2.2kb (gzipped is still 1.4kb).