Skip to content

Conversation

silverwind
Copy link
Member

Syntax highlighting is now done in a separate thread which should help performance on large files.

The web worker is initialized from a function which is not something IE11 supports and which would force us to create a separate .js file for the worker code.

For the reason above, I think it's a good opportunity to end IE11 support with this commit, so it should be landed in the next minor release version.

The code uses various ES2017 features. It should work in all evergreen browsers.

Fixes: #6147

Syntax highlighting is now done in a separate thread which should help
performance on large files.

The web worker is initialized from a function which is not something
IE11 supports and which would force us to create a separate .js file
for the worker code.

For the reason above, I think it's a good opportunity to end IE11 support
with this commit, so it should be landed in the next minor release
version.

The code uses various ES2017 features and should work in all evergreen
browsers.

Fixes: go-gitea#6147
@silverwind silverwind force-pushed the worker-hl branch 3 times, most recently from f8a32ab to ababfbf Compare August 3, 2019 13:09
@lafriks lafriks added pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! type/enhancement An improvement of existing functionality labels Aug 3, 2019
@lafriks lafriks added this to the 1.10.0 milestone Aug 3, 2019
@codecov-io
Copy link

codecov-io commented Aug 3, 2019

Codecov Report

Merging #7727 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7727      +/-   ##
==========================================
- Coverage   41.29%   41.29%   -0.01%     
==========================================
  Files         472      472              
  Lines       63833    63833              
==========================================
- Hits        26363    26360       -3     
- Misses      34032    34034       +2     
- Partials     3438     3439       +1
Impacted Files Coverage Δ
modules/highlight/highlight.go 31.81% <ø> (ø) ⬆️
modules/process/manager.go 76.81% <0%> (-4.35%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d9a130...3cadfcc. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 3, 2019
if (node.querySelector("ol.linenums")) {
const lines = result.split(/\r?\n/).map((line, i) => `<li class="L${i + 1}" rel="L${i + 1}">${line}</li>`);
node.classList.add("hljs");
node.innerHTML = `<ol class="linenums">${lines.join("")}<ol>`;
Copy link
Member Author

@silverwind silverwind Aug 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ol.linenums business seems to only be present in rendered files to support adding the yellow background on linked lines. Not happy having to do this wrapping here at all. I think future improvement would be to eliminate these children of pre > code so they would contain only text nodes and that the output of the highlighter can be rendered as-is in all cases.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@silverwind
Copy link
Member Author

silverwind commented Aug 3, 2019

There is a regression in multi-line highlights:

> hljs.highlightAuto("`a\nb`", ["javascript"]).value
"<span class=\"hljs-string\">`a
b`</span>"

Essentially result can not be split on newlines because it breaks the HTML structure. I guess I need to eliminate those ol.linenums wrapping inside pre > code for this to actually work.

@silverwind
Copy link
Member Author

Actually, I think I'll have to give up on this for now. Reasons being:

  • hightlight.js can not support HTML elements inside highlighted content without having access to DOM APIs. We can either eliminate HTML content inside the rendered code or offload DOM fixing to the main thread again which does not help performance.
  • there is not as much benefit of using a worker than I hoped. Essentially, even on the main thread, syntax highlighting is pretty fast. Worst cases like this file still highlight in about 10 seconds.

Generally, I think syntax highlighting should be performed server-side.

@silverwind silverwind closed this Aug 3, 2019
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider dropping IE 11 support
4 participants