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

Highlighting code with CRLF end-of-lines results in double line feeds in output when using Nunjucks #59

Closed
rmcginty opened this issue Aug 26, 2021 · 4 comments
Milestone

Comments

@rmcginty
Copy link
Contributor

This works fine in Markdown, but in Nunjucks, having \r\n at the end of a line results in two blank lines. Apparently Prism is outputting multiple \n instead of preserving the line ending which results in the Nunjucks template code adding an extra blank line for each line in the input.

Solution seems to be to remove all \r prior to highlighting. Submitting a PR.

@JKC-Codes
Copy link

JKC-Codes commented Aug 26, 2021

I've managed to recreate this issue by using either alwaysWrapLineHighlights: true or lineSeparator: "\r\n" as options.

I don't think the issue is with Prism. I think the issue is with the way we're splitting lines here:

let lines = highlightedContent.split("\n");
lines = lines.map(function(line, j) {
if(options.alwaysWrapLineHighlights || highlightNumbers) {
let lineContent = group.getLineMarkup(j, line);
return lineContent;
}
return line;
});
return `<pre class="language-${language}"${preAttributes}><code class="language-${language}"${codeAttributes}>` + lines.join(options.lineSeparator || "<br>") + "</code></pre>";

By only splitting on '\n' we're leaving a carriage return in the string. For example:
'foo\r\nbar'.split('\n') --> ['foo\r', 'bar']
Then when we're joining with the line separator we're getting:
['foo\r', 'bar'].join('<br>') --> 'foo\r<br>bar'

@rmcginty
Copy link
Contributor Author

Hmm. I am using alwaysWrapLineHighlights: false and lineSeparator: '<br>' and still see the issue. I was able to get the unit tests running. Let me see if I can get a couple of new tests that reproduce the issue.

@bennypowers
Copy link

Those waiting on the above-linked PR can apply this patch (which contains a bunch of irrelevant auto-formatting changes, sorry):

patches/@11ty+eleventy-plugin-syntaxhighlight+4.0.0.patch
diff --git a/node_modules/@11ty/eleventy-plugin-syntaxhighlight/src/HighlightPairedShortcode.js b/node_modules/@11ty/eleventy-plugin-syntaxhighlight/src/HighlightPairedShortcode.js
index 5dbd602..bc5765c 100644
--- a/node_modules/@11ty/eleventy-plugin-syntaxhighlight/src/HighlightPairedShortcode.js
+++ b/node_modules/@11ty/eleventy-plugin-syntaxhighlight/src/HighlightPairedShortcode.js
@@ -3,31 +3,32 @@ const PrismLoader = require("./PrismLoader");
 const HighlightLinesGroup = require("./HighlightLinesGroup");
 const getAttributes = require("./getAttributes");
 
-module.exports = function (content, language, highlightNumbers, options = {}) {
+module.exports = function(content, language, highlightNumbers, options = {}) {
   const preAttributes = getAttributes(options.preAttributes);
   const codeAttributes = getAttributes(options.codeAttributes);
 
   // default to on
-  if(options.trim === undefined || options.trim === true) {
+  if (options.trim === undefined || options.trim === true) {
     content = content.trim();
   }
 
   let highlightedContent;
-  if( language === "text" ) {
+  if ( language === "text" ) {
     highlightedContent = content;
   } else {
     highlightedContent = Prism.highlight(content, PrismLoader(language), language);
   }
 
-  let group = new HighlightLinesGroup(highlightNumbers);
-  let lines = highlightedContent.split("\n");
+  const group = new HighlightLinesGroup(highlightNumbers);
+  // see https://github.com/11ty/eleventy-plugin-syntaxhighlight/pull/60/files
+  let lines = highlightedContent.split(/\r?\n/);
   lines = lines.map(function(line, j) {
-    if(options.alwaysWrapLineHighlights || highlightNumbers) {
-      let lineContent = group.getLineMarkup(j, line);
+    if (options.alwaysWrapLineHighlights || highlightNumbers) {
+      const lineContent = group.getLineMarkup(j, line);
       return lineContent;
     }
     return line;
   });
 
-  return `<pre class="language-${language}"${preAttributes}><code class="language-${language}"${codeAttributes}>` + lines.join(options.lineSeparator || "<br>") + "</code></pre>";
+  return `<pre class="language-${language}"${preAttributes}><code class="language-${language}"${codeAttributes}>${lines.join(options.lineSeparator || "<br>")}</code></pre>`;
 };

@zachleat zachleat added this to the 4.1.0 milestone Jun 28, 2022
@zachleat
Copy link
Member

Shipping with 4.1.0!

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 a pull request may close this issue.

4 participants