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

New lines on code blocks get stripped when using rehype-prism #221

Closed
hugmanrique opened this issue Aug 12, 2018 · 8 comments
Closed

New lines on code blocks get stripped when using rehype-prism #221

hugmanrique opened this issue Aug 12, 2018 · 8 comments

Comments

@hugmanrique
Copy link
Contributor

hugmanrique commented Aug 12, 2018

New lines sometimes get stripped from the @mapbox/rehype-prism output when highlighting a code block.

I'm using the following Webpack loader:

{
  test: /\.mdx$/,
  include: path.resolve(__dirname, 'posts'),
  loader: [
    'babel-loader',
    {
      loader: '@mdx-js/loader',
      options: { hastPlugins: [[rehypePrism, { ignoreMissing: true }]] }
    }
  ]
}

The following .mdx file:

\`\`\`dockerfile
# Add main script
COPY start.sh /home/start.sh
COPY build-log.xml /home/playpen/

# GitLab CI doesn't have access to /home
RUN chmod -R 700 /home/
\`\`\`

And here's the result:
mdx-loader output

React source (produced by @mdx-js/loader): https://paste.ubuntu.com/p/C3cyx5ZYjV/

Which React renders as:

<pre><code class="language-dockerfile"><span class="token comment"># Add main script</span><span class="token keyword">COPY</span> start.sh /home/start.sh
<span class="token keyword">COPY</span> build<span class="token punctuation">-</span>log.xml /home/playpen/

<span class="token comment"># GitLab CI doesn't have access to /home</span><span class="token keyword">RUN</span> chmod <span class="token punctuation">-</span>R 700 /home/
</code></pre>

As you can see, there should be a line break between the first .token .comment (# Add main script) and the following .token .keyword (COPY).

One of the reasons why this could be happening is the @mdx-js/loader output is an array of react components, so newlines produced by rehype plugins might not be preserved. I haven't looked to much into it, so I might be wrong.

I also didn't know whether to report this on the rehype-prism repo or here, but I couldn't reproduce it with a barebones remark setup.

@silvenon
Copy link
Contributor

Hey! I've been away on holidays for a while so I need some time to catch up, but I just wanted to say thanks for being so excellent at reporting issues! ⭐️

@hugmanrique
Copy link
Contributor Author

Thank you! Also, don't worry about it, this is for a personal project so I'm not on a deadline 😀

@silvenon
Copy link
Contributor

@hugmanrique feel free to help out with this issue, I'm having trouble pinning down where the problem happens. 😕

@johno johno closed this as completed in e883828 Sep 4, 2018
@hugmanrique
Copy link
Contributor Author

hugmanrique commented Sep 4, 2018

Thank you for the quick fix! 😀

@phil-lgr
Copy link

phil-lgr commented Sep 6, 2018

I'm getting started with MDX, it's really nice so far, @silvenon are you interested in having example config for use with Prism.js or other syntax highlighting lib? We could have a section in the docs for that.

If you can drop the fix on npm when you have time that'd be appreciated :) thanks again!

@silvenon
Copy link
Contributor

silvenon commented Sep 6, 2018

Sure! There is just one thing I don't understand about @mapbox/rehype-prism. It places the .language- class on the code tag, while most Prism themes seem to count on the class to be on pre tag instead. Do you know why is that? In my personal project I inserted an additional rehype plugin to "correct" that, but I'm wondering why rehype-prism repo doesn't seem to mention this.

@davidtheclark
Copy link

Adding the class to the code tag is what is suggested by the Prism docs, which reference the HTML spec. I do see that if you inspect the Prism or MDN websites, both pre and code have the language class — which I assume is because when Prism executes at runtime it ends up messing with that. I'd be open to a PR adding the language class to both of the elements.

@silvenon
Copy link
Contributor

silvenon commented Sep 6, 2018

Thanks! We can continue tracking this in #253.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants