-
Notifications
You must be signed in to change notification settings - Fork 6.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
Update dependencies #1764
Update dependencies #1764
Conversation
package.json
Outdated
"handlebars": "4.0.11", | ||
"jstransformer-handlebars": "1.1.0", | ||
"junk": "2.1.0", | ||
"lodash.defaultsdeep": "4.6.0", | ||
"marked": "0.3.19", | ||
"marked": "^0.4.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Careful with this one. It may break stuff.
/ping @vsemozhetbyt @rubys
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I've ran npm run test
and everything seems going well with me. So I submitted here……
So what's wrong with that? Have you found something unstable or letting me removing "^"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we tried to update marked 0.3.5 to 0.4.0 in the main repo, it broke all sorts of things in our docs rendering. Granted, that's a bigger jump than is happening here, and the docs probably have more idiosyncratic and/or complicated markdown than in the website. I'm not sure if anything really breaks here. But I'm not sure it doesn't either. Maybe do a build with both 0.3.19 and 0.4.0 and use a diff tool to look at the results just to make sure nothing is obviously wrong? And/or scan through nodejs/node#21081 to get an idea of the types of things that broke for us in the docs? For example:
- tools: update marked to 0.4.0 node#21081 (comment)
- tools: update marked to 0.4.0 node#21081 (comment)
- tools: update marked to 0.4.0 node#21081 (comment)
...and more.
None of those were caught by automated test. It took @vsemozhetbyt painstakingly comparing the output from the two versions and finding these things.
Ultimately, in core, we decided to not update marked
and instead @rubys did a tremendous job removing marked
from core dependencies and replacing it with remark
. That said, we're still fixing a few bugs there. It was not a small undertaking.
marked
is a good lightweight package for markdown processing. It's not anywhere near fully spec-compliant and doesn't do much in the way of parsing. It relies heavily on a lot of complicated regular expressions instead. remark
seems to be more of a robust parser and I believe is spec-compliant (or close).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK,revert it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe put the marked
update in a separate pull request and people can look at just those changes carefully.
Update some dependencies in the package.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM if if tests are passing
Update some dependencies in the package.json as a general maintance.