Move default highlight.js highlighting into a web worker #3591
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3591 +/- ##
==========================================
- Coverage 36.16% 36.15% -0.01%
==========================================
Files 285 285
Lines 40905 40905
==========================================
- Hits 14792 14790 -2
- Misses 23941 23943 +2
Partials 2172 2172
Continue to review full report at Codecov.
|
tboerger
added
the
lgtm/need 2
|
I've fixed one of the codacy issues in 0d5afc2 (missing semi-colon) but I've left the other one (strings should use double quotes) since it seems at odds with the rest of the js files (they use single-quotes). Happy to change it if that's preferred though. |
public/js/index.js
Outdated
| @@ -1430,7 +1430,13 @@ $(document).ready(function () { | |||
|
|
|||
| // Highlight JS | |||
| if (typeof hljs != 'undefined') { | |||
| hljs.initHighlightingOnLoad(); | |||
| $('pre code').each(function (index, element) { | |||
| var worker = new Worker('/js/highlight-worker.js'); | |||
There was a problem hiding this comment.
Maybe we can only have one worker by putting it before the each.
There was a problem hiding this comment.
This will need to keep reference of element to update but will limit multiple worker loading
There was a problem hiding this comment.
public/js/highlight-worker.js
Outdated
| // A very simple web worker for highlight.js | ||
| // See: https://highlightjs.org/usage/ | ||
| onmessage = function(event) { | ||
| importScripts('/vendor/plugins/highlight/highlight.pack.js'); |
There was a problem hiding this comment.
It must be possible to put importScripts outside of onmessage.
There was a problem hiding this comment.
And you will have to follow codacy (even if it is stupid) to be able to be merge by maintainers. Maybe if you add
'use strict'; at start of file it may also not trigger for simple ' string
tboerger
added
lgtm/need 1
|
Caniuse stats. will work in 94.5% of cases, and it is supported in IE >= 10 so this looks alright. Except... initHighlightingOnLoad does not just get the text and highlight it accordingly - it correctly handles the case, like it happens in gitea, where there are children elements to the The reason why this needs to be addressed is that right now the code page looks like this:
with the changes of this pr, it will look like this (uglier):
This is not the only implication: selecting a line to get the link to it will also not work (will just append |
|
@thehowl - ah, good catch. I'm not sure I have a solution to that - Highlight.js' Sorry for wasting your time :( |
|
No time was wasted - we now know there is no easy solution for this, but this may still be necessary to add sometime in the future :) (Although I tend to think that we should rather try to move syntax highlighting to the backend, but I think there's already an issue for that somewhere) |
Hi,
We noticed that pages served by gitea locked up when viewing large files, which I tracked down to (mainly) the syntax highlighting javascript. Highlight.js have an example on their docs of moving the actual highlighting to a web worker so that it doesn't block the rest of the page, and this seems to be a big improvement for us so I thought I'd see whether it would be interesting to you.
There are a couple of other places where you use highlight js, but they're for specific situations (preview panes etc) where I wasn't so sure this would be desirable. I'd be happy to extend it to those too if needs be.
Thanks!