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

Links to internal anchors (or implicit anchors) cause error #45

Closed
nazrhyn opened this issue Aug 18, 2016 · 9 comments
Closed

Links to internal anchors (or implicit anchors) cause error #45

nazrhyn opened this issue Aug 18, 2016 · 9 comments

Comments

@nazrhyn
Copy link

nazrhyn commented Aug 18, 2016

In convert-md.js, when the renderer.link function is called on a link that looks like this:

1. [Connections](#connections)

Its call to url.parse produces something like this (assigned to parsed):

{
  protocol: null,
  slashes: null,
  auth: null,
  host: null,
  port: null,
  hostname: null,
  hash: '#connections',
  search: null,
  query: null,
  pathname: null,
  path: null,
  href: '#connections'
}

parsed.pathname is then passed to path.extname which fails with the following because pathname is null.

TypeError: Path must be a string. Received null

I use links like that to link to the implicit anchors provided by the ids generated for heading elements. Changing line 58 to...

if (!parsed.protocol && parsed.pathname) {

...fixes the problem. Not sure if that's more shotgun than you'd want, though.

@a7madgamal
Copy link

a7madgamal commented Sep 1, 2016

+1
i fixed it with
var ext = path.extname(parsed.pathname || '');
should i send a pr with this fix?

@a7madgamal
Copy link

I hope someone can comment on this and hopefully push a fix. it's almost blocking for us now!

@nazrhyn
Copy link
Author

nazrhyn commented Sep 7, 2016

I'd be happy to do a PR to resolve this, I just need some interaction to let me know what the maintainers would like.

@mixu
Copy link
Owner

mixu commented Sep 7, 2016

@nazrhyn sorry for the delay! A PR would be great!

@mixu mixu closed this as completed in edf3e50 Sep 7, 2016
@mixu
Copy link
Owner

mixu commented Sep 7, 2016

@nazrhyn I went ahead and fixed this, thanks @a7madgamal fir reporting and sorry for the delay again!

Published as v3.1.8

@a7madgamal
Copy link

thx all <3

@nazrhyn
Copy link
Author

nazrhyn commented Sep 7, 2016

@mixu I will say that I think that doing the || '' is a bit disingenuous. It is known, by the time url.parse is called, that there is no pathname. There's no reason to even execute path.extname or run the if statement below that.

/shrug

But that's just a stylistic point. Thanks for the fix!

@a7madgamal
Copy link

tbh I didn't look much into the code. I just tried to fix an error that was blocking a build and that's why I didn't send a pr with that hack :)

@a7madgamal
Copy link

assuming I was the inspiration for the fix :D

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

No branches or pull requests

3 participants