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 with "https://" in link text not properly parsed #8

Closed
starpit opened this issue Feb 3, 2021 · 5 comments
Closed

Links with "https://" in link text not properly parsed #8

starpit opened this issue Feb 3, 2021 · 5 comments
Labels
👀 no/external This makes more sense somewhere else 🙅 no/wontfix This is not (enough of) an issue for this project

Comments

@starpit
Copy link

starpit commented Feb 3, 2021

Subject of the issue

When using the remark-gfm plugin, certain links are misparsed. Without the plugin, links are always parse properly. Plugging the breaking input into github.com's Preview results in the expected correct parsing behavior. See below for an example.

Your environment

  • OS: any
  • Packages: react-markdown@5.0.3, remark-gfm@1.0.0
  • Env: Electron 10

Steps to reproduce

With the following input

import gfm from 'remark-gfm'
import ReactMarkdown from 'react-markdown'
/*...*/
<ReactMarkdown plugins={[gfm]} source="[https://google.com](https://google.com)"/>

the link is parsed such that the link callback has an odd href attribute of

https://google.com](https://google.com)

I checked, and that source is parsed as expected by github.com. If instead I use the following source, the link is now parsed properly by react-markdown:

[google.com](https://google.com)

And if i remove the gfm plugin, then both source are parsed properly.

Expected behavior

remark-gfm should not affect link parsing

Actual behavior

Link parsing seems to break when using remark-gfm.

@starpit starpit added 🐛 type/bug This is a problem 🙉 open/needs-info This needs some more info labels Feb 3, 2021
@starpit starpit changed the title Links not properly parsed Links with "https://" in link text not properly parsed Feb 3, 2021
@wooorm
Copy link
Member

wooorm commented Feb 3, 2021

did you rm -r node_modules/ && npm install again?

wooorm added a commit to syntax-tree/mdast-util-gfm-autolink-literal that referenced this issue Feb 3, 2021
@wooorm wooorm closed this as completed in 90a12bb Feb 3, 2021
@wooorm
Copy link
Member

wooorm commented Feb 3, 2021

I added several tests and can’t reproduce it.

@wooorm wooorm added 🙅 no/wontfix This is not (enough of) an issue for this project and removed 🐛 type/bug This is a problem 🙉 open/needs-info This needs some more info labels Feb 3, 2021
@starpit
Copy link
Author

starpit commented Feb 3, 2021

The problem here is that our package-lock.json was specifying mdast-util-gfm@0.1.1. The bug seems to have been fixed in 0.1.2 of that module. Thus, even though there was no later version of the top-level modules (i.e. both react-markdown and remark-gfm versions haven't changed), our application was unable to pick up the bug fix, without a complete re-generation of our package-lock.json. This is dangerous, and so we avoid it as much as possible.

It might be nice to consider specifying a minimum version of mdast-util-gfm on your side, given that this bug manifests at the remark-gfm level with the dependence you currently specify:

    "mdast-util-gfm": "^0.1.0"

(in my opinion, this bug isn't fixed until you update your package.json?)

thanks!

@wooorm
Copy link
Member

wooorm commented Feb 3, 2021

This is what the caret stands for: accept bug fixes. Before we had package locks, we had semver.

@ChristianMurphy
Copy link
Member

ChristianMurphy commented Feb 3, 2021

our application was unable to pick up the bug fix, without a complete re-generation of our package-lock.json. This is dangerous, and so we avoid it as much as possible.

🤷‍♂️ this goes both ways, yes occasionally upgrading the lockfile may cause a regression, the majority of the time it's bug fixes.
Which is why dependabot, renovate, and others exist to structure, manage, and ensure stability in the process.

in my opinion, this bug isn't fixed until you update your package.json

As @wooorm mentions, that is what a version range does, it allows smooth upgrades.
If you want to do a more selective upgrade https://dev.to/malykhinvi/fix-a-transitive-npm-dependency-vulnerability-4775 offers a path for upgrading one transitive dependency.
Though in general I'd recommend upgrading all transitive dependencies.

@ChristianMurphy ChristianMurphy added the 👀 no/external This makes more sense somewhere else label Feb 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👀 no/external This makes more sense somewhere else 🙅 no/wontfix This is not (enough of) an issue for this project
Development

No branches or pull requests

3 participants