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

The markdown table is parsed unexpectedly in css-loader documentation page #1187

Closed
Alex1990 opened this issue May 3, 2017 · 11 comments
Closed
Assignees

Comments

@Alex1990
Copy link

Alex1990 commented May 3, 2017

In the "Options" section of the css-loader documentation.

image

@skipjack
Copy link
Collaborator

skipjack commented May 3, 2017

@Alex1990 thanks for pointing this out!
@NejcZdovc can you look into this? If you're busy, let me know and I can dig into it.

@skipjack skipjack added the Bug label May 3, 2017
@NejcZdovc
Copy link
Collaborator

@skipjack problem here is that we are not escaping | correctly, that's why new column is added.

This is a markdown from css-loader repo:
image

As you can see {Boolean\|Object} is in there.

@skipjack
Copy link
Collaborator

skipjack commented May 5, 2017

Hmmm after looking at the markdown utility I don't see where the parsing fails. Are we manually parsing each table line somewhere?

@NejcZdovc
Copy link
Collaborator

For table we have a custom handler and it starts here https://github.com/webpack/webpack.js.org/blob/master/utilities/markdown.js#L102. Thing is that this is parsed and handled by markdown util, we just add some wrappers around it.

@skipjack
Copy link
Collaborator

skipjack commented May 9, 2017

Thing is that this is parsed and handled by markdown util, we just add some wrappers around it.

Yea that's what I noticed when going through it as well. It doesn't seem like we're manually calling .split(' | ') or anything similar anywhere. I wonder if this was broken by something else were doing like the reference links.

@NejcZdovc
Copy link
Collaborator

maybe it's bug in markdown?

@skipjack
Copy link
Collaborator

skipjack commented May 9, 2017

I don't think so because is parsed correctly in github. Maybe in marked but I doubt it, seeing as the reference link issue is definitely caused by our customizations.

I really want to move to markdown-to-react-components but don't have the time yet to do the port (I think it'd take a decent amount of effort).

@skipjack
Copy link
Collaborator

Not pretty, but I came up with a temp solution. I'll PR it tonight.

After digging into it a bit more I'm still unsure as to what we're doing that's breaking this and other stuff (like reference links). I tried commenting some bits out, but nothing seem to fix the odd behaviors. This could be something that just an issue in marked, the only way to find out would be to test a similar table using marked without any customizations.

@skipjack skipjack self-assigned this May 10, 2017
@Alex1990
Copy link
Author

Alex1990 commented May 10, 2017

Okay, I find it is a marked's bug markedjs/marked#595. And, I use the 0.3.6(latest) marked to test it.

@Alex1990
Copy link
Author

Should we use <code>{Object&verbar;Boolean}</code> which is parsed rightly by github and marked?

@skipjack
Copy link
Collaborator

Okay, I find it is a marked's bug markedjs/marked#595. And, I use the 0.3.6(latest) marked to test it.

@Alex1990 awesome thanks for digging into that. Unfortunately, reading into that issue doesn't give me much hope that it'll get fixed. I think we just need to switch our markdown parsing process.

Should we use {Object|Boolean} which is parsed rightly by github and marked?

The content for that page is actually from another repository. I think it'd be tough to enforce people not to use things that are supported by GFM. We probably need to make sure that when we do update our markdown processing that we fully support GFM if possible.

I do have a fix for the meantime I'll add PR for shortly.

skipjack added a commit that referenced this issue May 10, 2017
This is allowed in GFM but not supported by marked. See this issue:
markedjs/marked#595

Fixes #1187
skipjack added a commit that referenced this issue May 12, 2017
…nt (#1204)

This is allowed in GFM but not supported by marked. See this issue:
markedjs/marked#595

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

No branches or pull requests

3 participants