-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Markdown: Fixed markdown not working in NodeJS #2977
Conversation
This is not a regression, right? If not, why is it urgent? |
What introduced this regression? |
The linked commit did introduce the regression. We just haven't had a release for 6 months, so it went unnoticed. |
I see. I'm not a huge fan of stripping HTML with regexes (it worries me about potential security issues or incorrect results) but I suppose our best bet is to merge this in and remove the regression as soon as possible, while we figure out a better long-term fix. |
Neither am I. I'll open a separate issue for a sound way to get the text content of an HTML string. |
@LeaVerou, can I take your comment as approval? Just want to make sure :) I plan on releasing a patch right after merging this. |
Actually, I want the patch to include a fix for #2974 as well. |
@RunDevelopment If #2974 could not be merged soon, would you mind to release a patch for this fix first? |
@meteorlxy v1.24.1 is now available. It includes both fixes. |
Markdown crashed on Node. The code-block-highlighting relyed on browser DOM to get the text content of an HTML string (see
wrap
hook function).The fix employed by this PR is to replace the DOM-based approach with a custom function:
textcontent(html: string)
. The function is implemented using Prism's markup tag regex and some custom logic to unescape HTML entities I stole (not literally) from lowdash. It works pretty well on Node and in browsers (notably IE11).I also contemplated detecting whether we have DOM access and using the DOM approach when available. I decided against this because:
@mAAdhaTTah @LeaVerou @Golmote
This fix is urgent. Please review ASAP.
I want to release the fix right after it got merged.
This fixes #2973.