Skip to content

Conversation

@rubys
Copy link
Member

@rubysrubys commented Aug 5, 2018

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
Copy link
Collaborator

@nodejs-github-botnodejs-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
Copy link
Contributor

vsemozhetbyt commented Aug 5, 2018

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
Copy link
Contributor

vsemozhetbyt commented Aug 5, 2018

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:

11
12

21
22

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Aug 5, 2018

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:

3

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:

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

@rubys
Copy link
MemberAuthor

rubys commented Aug 5, 2018

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

@vsemozhetbytvsemozhetbyt mentioned this pull request Aug 7, 2018
4 tasks
@rubys
Copy link
MemberAuthor

rubys commented Aug 8, 2018

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
Copy link
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
Copy link
MemberAuthor

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

rubys added a commit to rubys/node that referenced this pull request Aug 13, 2018
Preferred long term fix can be found at: nodejs#22140
rubys added a commit that referenced this pull request Aug 13, 2018
quick fix for #22065 Preferred long term fix can be found at: #22140 PR-URL: #22084Fixes: #22065 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
rvagg pushed a commit that referenced this pull request Aug 15, 2018
quick fix for #22065 Preferred long term fix can be found at: #22140 PR-URL: #22084Fixes: #22065 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
@jasnell
Copy link
Member

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

@trivikr
Copy link
Member

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

@Trott
Copy link
Member

Trott commented Dec 5, 2018

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

@TrottTrottforce-pushed the mdast-util-to-hast-302 branch from 8f1c6ab to 6118207CompareDecember 14, 2018 05:09
@Trott
Copy link
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
Copy link
Member

@BridgeARBridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 10, 2019
@refackrefackforce-pushed the mdast-util-to-hast-302 branch from 6118207 to ade009eCompareMarch 10, 2019 01:48
@refack
Copy link
Contributor

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 <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
@refackrefackforce-pushed the mdast-util-to-hast-302 branch from ade009e to cba2c7aCompareMarch 10, 2019 14:27
@refackrefack merged commit cba2c7a into nodejs:masterMar 10, 2019
@refackrefack removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 10, 2019
BridgeAR pushed a commit that referenced this pull request Mar 13, 2019
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 <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
BridgeAR pushed a commit that referenced this pull request Mar 13, 2019
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 <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
BridgeAR pushed a commit that referenced this pull request Mar 14, 2019
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 <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
BethGriggs pushed a commit that referenced this pull request Apr 16, 2019
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 <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
@BethGriggsBethGriggs mentioned this pull request May 1, 2019
firass111 pushed a commit to firass111/Project_node1 that referenced this pull request Apr 16, 2025
quick fix for #22065 Preferred long term fix can be found at: nodejs/node#22140 PR-URL: nodejs/node#22084Fixes: nodejs/node#22065 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docIssues and PRs related to the documentations.toolsIssues and PRs related to the tools directory.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

doc: header escaping regression

8 participants

@rubys@nodejs-github-bot@vsemozhetbyt@Trott@jasnell@trivikr@BridgeAR@refack