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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Escaped pipes are ignored within table cells #258

Closed
robaxelsen opened this issue Jul 25, 2019 · 7 comments 路 Fixed by #261, BKWLD/vue-height-tween-transition#6 or kspramod/pacman#4
Closed

Comments

@robaxelsen
Copy link

Hi 馃憢 馃槃

I am running into this issue in Storybook, which uses markdown-to-jsx for markdown parsing: storybookjs/storybook#6869

Could not find any issues here regarding this, so reporting it. Would it be possible to fix this? Thanks in advance for any help!

@quantizor
Copy link
Owner

Yeah, open to PRs 馃憤

@robaxelsen
Copy link
Author

Looking deeper into this, I see the same output when parsing a table with escaped pipe characters with react-native-simple-markdown as well. And both seem to be built on top of simple-markdown. Will close this for now, and report there.

@quantizor quantizor reopened this Jul 31, 2019
@quantizor
Copy link
Owner

Reopened - this library has diverged a lot from simple-markdown so good to fix this separately.

@robaxelsen
Copy link
Author

Reopened - this library has diverged a lot from simple-markdown so good to fix this separately.

@probablyup Ok, thanks. I tried to look into the code, but was not able to get the full picture. Would you be able to point me to any logic involved for this specific case, or hints on how you would go about making a fix for this?

Here is the new issue I created, which also mentions my crude test repo I've been playing with to solve this: https://github.com/robaxelsen/markdown-to-jsx-table-pipe-test

@quantizor
Copy link
Owner

I think probably here: https://github.com/probablyup/markdown-to-jsx/blob/master/index.js#L162

Checking and rejecting pipes that have an escape

@robaxelsen
Copy link
Author

robaxelsen commented Aug 1, 2019

I think probably here: https://github.com/probablyup/markdown-to-jsx/blob/master/index.js#L162

Checking and rejecting pipes that have an escape

@probablyup Thanks! I did some testing, and seems that line is used to match any line that is part of a table markdown.

This one seems promising though, as it's used throughout the code to split the row into cells, if I am reading the code correctly: https://github.com/probablyup/markdown-to-jsx/blob/5d4470314de8d652435e581851e4e8e2ef78a6c0/index.js#L174

Tried replacing it with regex that should ignore the escaped pipe, but having issues getting it to match. Have any ideas?

Posted a relevant stackoverflow question here, which might help shed some more light on this: https://stackoverflow.com/questions/57306043/splitting-complex-string-with-while-ignoring-escaped-delimeter

@ariabuckles
Copy link
Collaborator

ariabuckles commented Aug 5, 2019

In simple-markdown I was able to fix this by actually parsing each row, rather than using String.prototype.split. The patch is here: ariabuckles/simple-markdown@f32a628 , and I might try to see how hard it would be to do the same thing here.

Edit: sent you a PR: #261

ariabuckles added a commit to ariabuckles/markdown-to-jsx that referenced this issue Aug 5, 2019
Should fix quantizor#258

A port of ariabuckles/simple-markdown@f32a628
to markdown-to-jsx.

Previously, tables were parsed using String.prototype.split on pipe
(`|`) characters.

This commit changes table parsing to use a full parse() call, using a
new `tableSeparator` rule to match tableSeparators. Because
`tableSeparator` is now a rule, the `escapedText` rule can handle
escaped pipes within the table, and the `codeInline` rule can handle
inline code with pipes in it.

Test plan:

Added tests to index.compiler.spec.js
quantizor pushed a commit that referenced this issue Aug 6, 2019
Should fix #258

A port of ariabuckles/simple-markdown@f32a628
to markdown-to-jsx.

Previously, tables were parsed using String.prototype.split on pipe
(`|`) characters.

This commit changes table parsing to use a full parse() call, using a
new `tableSeparator` rule to match tableSeparators. Because
`tableSeparator` is now a rule, the `escapedText` rule can handle
escaped pipes within the table, and the `codeInline` rule can handle
inline code with pipes in it.

Test plan:

Added tests to index.compiler.spec.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants