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

Adds server-side code block formatting #442

Closed
wants to merge 3 commits into from
Closed

Adds server-side code block formatting #442

wants to merge 3 commits into from

Conversation

phil-scott-78
Copy link
Contributor

This applies the highlight.js highlighting on the server allowing the client side script to be dropped as mentioned in #348. It does this using the MsieJavaScriptEngine to execute the script. The script itself is actually the node version, but I applied browserify to it so that it wouldn't require node anymore. Kind of a hack, but it works. I included the steps I took to build the file in regenerating-highlight-all.js so that it could be updated in the future with a newer version.

Right now the main thing it is missing is the ability to add languages and some of the other highlight.js configuration options, but I'm not sure how many people are using it. Wanted to get this out there and get some feedback before going totally overboard with options. I noticed mermaid has an NPM package too - https://www.npmjs.com/package/mermaid. Potentially it could be applied in a similar fashion. I might give that a go too, but I'm not terribly familiar with that library.

One thing this did bring to light is how to slam a new module into an existing pipeline. I.E. I wanted to run this before the Razor module in the Posts pipeline so right now I have this ugly code

Pipelines[BlogPipelines.Posts].Insert(1, new Highlight());

Might be cool if the Pipeline allowed something like

Pipelines[BlogPipelines.Posts].InsertBeforeFirst<Razor>(new Highlight());

or

Pipelines[BlogPipelines.Posts].InsertAfterLast<Razor>(new Highlight());

This applies the highlight.js highlighting on the server allowing the client side script to be dropped. It does this using the MsieJavaScriptEngine to execute the script. The script itself is actually the node version, but I applied browserify to it so that it wouldn't require node anymore. Kind of a hack, but it works. I included the steps I took to build the file in regenerating-highlight-all.js so that it could be updated in the future with a newer version.
@phil-scott-78
Copy link
Contributor Author

Mermaid might be a bit trickier. Seems they have some outstanding issues tied to it

mermaid-js/mermaid#260
mermaid-js/mermaid#169

@daveaglick
Copy link
Member

This is awesome - I didn't get a chance to look at it today, but will very soon.

Also, I love

Pipelines[BlogPipelines.Posts].InsertBeforeFirst<Razor>(new Highlight());

I've actually just been struggling with this myself on a large site and this would make it much easier (and more stable since an index would change if the underlying pipeline does). I'll open a separate issue for that addition (or you can if you beat me to it).

@phil-scott-78
Copy link
Contributor Author

@daveaglick - give me a bit on this one. I'm not a huge fan of having to build up a new MsieJsEngine for each document so I'm going to tweak it a bit to be more efficient. Running benchmarks right now for a baseline.

MsieJsEngine is certainly not thread safe so I was rebuildingi t from scratch within the loop. No sense in doing that, so switch the code to use a ThreadLocal instance to pool the JS instances between threads.  Not a huge win for small sites, but for a 1000 document test run it went from 15s to 1s on my machine.
@phil-scott-78
Copy link
Contributor Author

I had an interesting issue the MsieJsEngine not liking being manually disposed, but only in AppVeyor. Going to investigate their library, but for now I've just removed the explicit Dispose for now.

As to using ThreadLocal<T>, for a 1000 document site this went from 11s to 1s on my machine.

@daveaglick
Copy link
Member

I have the same concerns with this PR as I have with #439. I'm not sure MsieJavaScriptEngine gives us a good path forward for netstandard and cross platform compatibility. I wonder if the same approach used here with MsieJavaScriptEngine could be applied with Jint instead, and how big an effort that would be?

Adds a test where the code block doesn't have a language and highlightAuto
must be called. This not only covers that scenario but is an important
test case for javascript execution engines. Because highlight.js doesn't
know the language it must iterate through the languages it is configured
with and check them one by one using regular expressions. These have a
wide variety of implementations so it actually provides a good test case
for a javascript languages regex support. It seems anything doing JS -> IL
starts to fall short here due to the differences in supported Regex
escaping and language features. And with regex being a huge part of
linting and text manipulation supporting regex is a vital part of
evaluating a JS engine.
@phil-scott-78
Copy link
Contributor Author

closing because #452 did this better.

@phil-scott-78 phil-scott-78 deleted the add-highlightjs-server-side-module branch March 4, 2017 04:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants