FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

tools: update to mdast-util-to-hast v3.0.2 by rubys · Pull Request #22140 · nodejs/node · GitHub

/ node Public

tools: update to mdast-util-to-hast v3.0.2 - #22140

Merged
refack merged 1 commit into
nodejs:masterfrom
rubys:mdast-util-to-hast-302
Mar 10, 2019
ConversationCommits (1)ChecksFiles changed
Merged

tools: update to mdast-util-to-hast v3.0.2#22140
refack merged 1 commit into
nodejs:masterfrom
rubys:mdast-util-to-hast-302

Conversation

rubys commented Aug 5, 2018

Copy link
Copy Markdown
Member

See syntax-tree/mdast-util-to-hast#21

Fixes #22065

Note: I updated all of the tools/doc dependencies, not just this one,
and removed the previous workaround that was in place until this change
landed.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

nodejs-github-bot commented Aug 5, 2018

Copy link
Copy Markdown
Collaborator

@rubys build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/436/pipeline

nodejs-github-bot added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels Aug 5, 2018

vsemozhetbyt commented Aug 5, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

There is good news.

This PR fix two known regressions and one previously unknown regression:

  1. Regression with Markdown escapes described in doc: header escaping regression #22065

  2. Regression with HTML entities described in doc: fix header escaping regression (Issue #22065) #22084 (comment)

  3. Unknown regression with Markdown backticks rendered in headings instead of code tags (nightly example).

vsemozhetbyt commented Aug 5, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

There is bad news.

This PR introduces one new regression. If a heading has two consecutive optional parameters in square brackets, they are not rendered as links, but some preprocess cleaning is done as if they are links:

  1. spaces between them are deleted;
  2. all text in the second pair of brackets is lowercased.

Compare these examples before and after this PR:

vsemozhetbyt commented Aug 5, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

And there is good bad news)

Comparing the diff helps to find out one issue that is differently exposed before and after this PR.

  1. Before this PR. If a parameter type signature is a 2-dimensional array with a [][] part, this part is rendered as an invisible empty link (nightly example, see records parameter) and the type is not properly shown and linkified.

  2. After this PR. All the type is rendered wrongly verbatim without proper linkification:

Compare the doc from the old toolchain: https://nodejs.org/docs/latest-v8.x/api/dns.html#dns_dns_resolvetxt_hostname_callback

One of the concerned code fragments:

node/tools/doc/type-parser.js

Lines 138 to 141 in 28c70ac

// To support type[], type[][] etc., we store the full string
// and use the bracket-less version to lookup the type URL.
const typeTextFull = typeText;
typeText = typeText.replace(arrayPart, '');

rubys commented Aug 5, 2018

Copy link
Copy Markdown
Member Author

I know what's going on. I'll try to see if i can create another patch for mdast-util-to-hast.

vsemozhetbyt mentioned this pull request Aug 7, 2018
Closed
4 tasks

rubys commented Aug 8, 2018

Copy link
Copy Markdown
Member Author

Three pull requests created to solve (most) of the problems noted above:

The 2-dimensional array appears to be a separate problem in that remark, et. al., appears to be doing the right thing. Once the above fixes land, I'll look into fixing that problem too.

Trott commented Aug 10, 2018

Copy link
Copy Markdown
Member

Should this update the corresponding package.json file and not just the package-lock.json? I mean, I guess that's not required, but if we want to be sure we are bumping a dependency up to a certain minimum version, that should be reflected in the package.json?

rubys commented Aug 10, 2018

Copy link
Copy Markdown
Member Author

@Trott will do... once the above three patches are resolved. But first, there are yaks that need shaving: syntax-tree/mdast#23 (comment)

vsemozhetbyt mentioned this pull request Aug 11, 2018
Closed
3 tasks
rubys mentioned this pull request Aug 12, 2018
Closed
2 tasks
rubys added a commit to rubys/node that referenced this pull request Aug 13, 2018
squash: quick fix for < and >
e956f6b 
Preferred long term fix can be found at:
nodejs#22140
rubys added a commit that referenced this pull request Aug 13, 2018
tools: fix header escaping regression
59f8276 
quick fix for #22065

Preferred long term fix can be found at:
#22140

PR-URL: #22084
Fixes: #22065
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
rvagg pushed a commit that referenced this pull request Aug 15, 2018
tools: fix header escaping regression
df7d33e 
quick fix for #22065

Preferred long term fix can be found at:
#22140

PR-URL: #22084
Fixes: #22065
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

jasnell commented Sep 10, 2018

Copy link
Copy Markdown
Member

Ping @rubys ... what's the status on this one?

trivikr commented Oct 1, 2018

Copy link
Copy Markdown
Member

The PRs listed in #22140 (comment) need a follow-up
Ping @rubys

Trott commented Dec 5, 2018

Copy link
Copy Markdown
Member

@rubys Looks like all upstream yaks have been shaved and this can be rebased, run through CI, and hopefully landed?

Trott force-pushed the mdast-util-to-hast-302 branch from 8f1c6ab to 6118207 Compare December 14, 2018 05:09

Trott commented Dec 14, 2018

Copy link
Copy Markdown
Member

Took the liberty of rebasing. Hope that's OK. Still have to install the upstream dependencies that have been updated and run through CI?

BridgeAR commented Mar 10, 2019

Copy link
Copy Markdown
Member

CI https://ci.nodejs.org/job/node-test-pull-request/21382/

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 10, 2019
refack force-pushed the mdast-util-to-hast-302 branch from 6118207 to ade009e Compare March 10, 2019 01:48

refack commented Mar 10, 2019

Copy link
Copy Markdown
Contributor

CI: https://ci.nodejs.org/job/node-test-pull-request/21385/

tools: update to mdast-util-to-hast v3.0.2
cba2c7a 
See syntax-tree/mdast-util-to-hast#21

Note: I updated all of the tools/doc dependencies, not just this one,
and removed the previous workaround that was in place until this change
landed.

PR-URL: nodejs#22140
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
refack force-pushed the mdast-util-to-hast-302 branch from ade009e to cba2c7a Compare March 10, 2019 14:27
refack merged commit cba2c7a into nodejs:master Mar 10, 2019
refack removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 10, 2019
BridgeAR pushed a commit that referenced this pull request Mar 13, 2019
tools: update to mdast-util-to-hast v3.0.2
d1df1c6 
See syntax-tree/mdast-util-to-hast#21

Note: I updated all of the tools/doc dependencies, not just this one,
and removed the previous workaround that was in place until this change
landed.

PR-URL: #22140
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BridgeAR pushed a commit that referenced this pull request Mar 13, 2019
tools: update to mdast-util-to-hast v3.0.2
7dd213f 
See syntax-tree/mdast-util-to-hast#21

Note: I updated all of the tools/doc dependencies, not just this one,
and removed the previous workaround that was in place until this change
landed.

PR-URL: #22140
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BridgeAR mentioned this pull request Mar 13, 2019
Merged
BridgeAR pushed a commit that referenced this pull request Mar 14, 2019
tools: update to mdast-util-to-hast v3.0.2
9d92887 
See syntax-tree/mdast-util-to-hast#21

Note: I updated all of the tools/doc dependencies, not just this one,
and removed the previous workaround that was in place until this change
landed.

PR-URL: #22140
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BethGriggs pushed a commit that referenced this pull request Apr 16, 2019
tools: update to mdast-util-to-hast v3.0.2
700df16 
See syntax-tree/mdast-util-to-hast#21

Note: I updated all of the tools/doc dependencies, not just this one,
and removed the previous workaround that was in place until this change
landed.

PR-URL: #22140
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BethGriggs mentioned this pull request May 1, 2019
Merged
This was referenced May 29, 2019
Closed
Closed
Closed
Closed
Closed
aliceUnhinged613 mentioned this pull request May 16, 2022
Open
This was referenced May 16, 2022
Open
Open
kissofire mentioned this pull request May 16, 2022
Open
wedataintelligence mentioned this pull request May 16, 2022
Open
ron-a20 mentioned this pull request May 16, 2022
Open
molllyn1 mentioned this pull request May 17, 2022
Open
firass111 pushed a commit to firass111/Project_node1 that referenced this pull request Apr 16, 2025
tools: fix header escaping regression
6cd2d1d 
quick fix for #22065

Preferred long term fix can be found at:
nodejs/node#22140

PR-URL: nodejs/node#22084
Fixes: nodejs/node#22065
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
BridgeAR BridgeAR approved these changes

Assignees

No one assigned

Labels

doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

doc: header escaping regression

8 participants

Back | FazBrowse Home | New Git URL