doc: fix header escaping regression (Issue #22065)#22084

rubys · 2018-08-02T11:02:46Z

Remove backslashes in headers.

Fixes: #22065

Checklist

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

vsemozhetbyt · 2018-08-02T14:51:52Z

tools/doc/html.js

Maybe just:
.replace(/\\(?=.)/g,''),

Or, if we need to exclude escaped escapes:
.replace(/\\(?=[^\\])/g,''),

jslint doesn't accept .replace(/\\(?=.)/g, ''),:
/Users/rubys/git/node/tools/doc/html.js 202:29 error Unescaped dot character in regular expression node-core/no-unescaped-regexp-dot
I want to include escaped escapes.
Finally, lookahead isn't appropriate here. If we have an escaped escape, I want to emit a single backslash and then to not consider that backslash as an escape character. Consider four escapes in a row, the desired result would be two backslashes:
> "\\\\\\\\".replace(/\\(?=.)/g, '').length 1 > "\\\\\\\\".replace(/\\.{1}/g, (match) => match[1]).length 2

What about .replace(/\\(.{1})/g, '$1'),? Whould it be simpler and faster then function call and string index access?

BTW, we can disable linter rule for a line to simplify RegExp, but I am not sure what is more confusing:
// eslint-disable-next-line node-core/no-unescaped-regexp-dot.replace(/\\./g,(match)=>match[1]),

vsemozhetbyt · 2018-08-02T16:53:42Z

CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/404/

vsemozhetbyt · 2018-08-02T17:03:44Z

Not sure what is wrong with CI. Is it infrastructural fails?

rubys · 2018-08-02T18:29:27Z

+ sed '/^/\s*$/d' sed: 1: "/^/\s*$/d": invalid command code \

Somebody either changed the version of sed or changed the build script. So, yes, I would say this looks like infrastructure fails.

Between the two, I would say that changing the build script seems considerably more likely. The second slash just looks outright wrong. Perhaps they meant to have a backslash escaping the next backslash so that they shell would pass sed a regular expression containing only a single backslash. If that is indeed the problem, it would seem amusing given the change that this particular pull request is addressing. :-)

Trott · 2018-08-02T21:57:23Z

Replay: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/408/

richardlau · 2018-08-03T03:24:17Z

Looks to be a real failure:

17:58:34 /data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/tools/doc/html.js:203 17:58:34 position 17:58:34 ^^^^^^^^ 17:58:34 17:58:34 SyntaxError: Unexpected identifier 17:58:34 at new Script (vm.js:72:7) 17:58:34 at createScript (vm.js:234:10) 17:58:34 at Object.runInThisContext (vm.js:286:10) 17:58:34 at Module._compile (internal/modules/cjs/loader.js:657:28) 17:58:34 at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10) 17:58:34 at Module.load (internal/modules/cjs/loader.js:599:32) 17:58:34 at tryModuleLoad (internal/modules/cjs/loader.js:538:12) 17:58:34 at Function.Module._load (internal/modules/cjs/loader.js:530:3) 17:58:34 at Module.require (internal/modules/cjs/loader.js:637:17) 17:58:34 at require (internal/modules/cjs/helpers.js:20:18) 17:58:34 Building addon in /data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/addons/05_wrapping_c_objects 17:58:34 make[2]: *** [out/doc/api/path.html] Error 1

https://ci.nodejs.org/job/node-test-commit-linuxone/3483/nodes=rhel72-s390x/consoleFull

richardlau · 2018-08-03T03:24:35Z

tools/doc/html.js

Missing trailing comma?

vsemozhetbyt · 2018-08-03T06:19:34Z

CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/411/

vsemozhetbyt · 2018-08-03T06:34:10Z

Node.js Collaborators, please, add 👍 here if you approve fast-tracking.

vsemozhetbyt · 2018-08-03T21:56:22Z

It seems we have one more similar regression case: HTML entities verbatim rendering:

https://nodejs.org/download/nightly/v11.0.0-nightly20180803d68f946fe4/docs/api/inspector.html#inspector_event_lt_inspector_protocol_method_gt

Not sure if this should be fixed in this PR or in a new one.

jasnell · 2018-08-03T22:52:46Z

@rubys ... just a nit the prefix for the commit message here should be tools: rather than doc:

Also, just as a convention, it is helpful to include a [Squash] or [Fixup] prefix on commits that need to be squashed prior to landing.

rubys · 2018-08-05T17:55:35Z

My pull request for https://www.npmjs.com/package/mdast-util-to-hast has been accepted. Updating to the latest version of this dependency should fix everything. In fact, I should be able to back out some of the work that is currently committed. I'll run some tests and see what needs to be done from there.

jasnell · 2018-08-12T17:10:58Z

@rubys ... is this ready to go?

rubys · 2018-08-12T18:37:31Z

@jasnell: this PR is an accumulated set of workarounds to a remark bug, it certainly can go in now, but will all need to be backed out when these pull requests land: #22140 (comment)

fixesnodejs#22065

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

rubys · 2018-08-13T01:00:06Z

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

rubys · 2018-08-13T05:47:01Z

My read is that the freebsd failure is unrelated. Unless there are any objections, I'll land this PR tomorrow.

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]>

rubys · 2018-08-13T15:16:43Z

Landed in 59f8276

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]>

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]>

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

vsemozhetbyt reviewed Aug 2, 2018
View reviewed changes

vsemozhetbyt approved these changes Aug 2, 2018
View reviewed changes

This comment has been minimized.
Sign in to view

richardlau reviewed Aug 3, 2018
View reviewed changes

tools/doc/html.js Outdated
Copy link
Member
richardlauAug 3, 2018
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing trailing comma?

vsemozhetbyt added fast-track PRs that do not need to wait for 48 hours to land. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Aug 3, 2018

jasnell changed the title ~~Issue 22065~~doc: fix header escaping regression (Issue #22065)Aug 3, 2018

jasnell approved these changes Aug 3, 2018
View reviewed changes

lpinca mentioned this pull request Aug 4, 2018
doc: headerlines not rendered correctly with extra backslash #22123
Closed

vsemozhetbyt mentioned this pull request Aug 5, 2018
tools: update to mdast-util-to-hast v3.0.2 #22140
Merged
2 tasks

trivikr approved these changes Aug 7, 2018
View reviewed changes

rubys mentioned this pull request Aug 7, 2018
Use \ for escaping in markdown #22169
Closed
4 tasks

rubys added 4 commits August 12, 2018 18:08

doc: fix header escaping regression
255654d
fixesnodejs#22065

appease linter
d0d6ef7

replace function with capture and backreference
925f0eb

add back in missing comma (DOH!)
4c6e3e0

squash: quick fix for < and >
e956f6b
Preferred long term fix can be found at: nodejs#22140

rubys force-pushed the issue-22065 branch from c9e7083 to e956f6bCompare August 13, 2018 00:09

trivikr approved these changes Aug 13, 2018
View reviewed changes

rubys closed this Aug 13, 2018

rvagg mentioned this pull request Aug 15, 2018
Release proposal: v10.9.0 #22295
Merged

aliceUnhinged613 mentioned this pull request May 16, 2022
[Snyk] Security upgrade gatsby from 2.18.18 to 2.19.36 aliceUnhinged613/node#58
Open

This was referenced May 16, 2022
[Snyk] Security upgrade gatsby from 2.18.18 to 2.19.36 137717unity/node#57
Open
[Snyk] Security upgrade gatsby from 2.18.18 to 2.19.36 Rednickle/node-1#49
Open

kissofire mentioned this pull request May 16, 2022
[Snyk] Security upgrade gatsby from 2.13.73 to 2.19.36 kissofire/node#67
Open

wedataintelligence mentioned this pull request May 16, 2022
[Snyk] Security upgrade gatsby from 2.18.18 to 2.19.36 wedataintelligence/node#33
Open

ron-a20 mentioned this pull request May 16, 2022
[Snyk] Security upgrade gatsby from 2.18.18 to 2.19.36 ron-a20/node#66
Open

molllyn1 mentioned this pull request May 17, 2022
[Snyk] Security upgrade gatsby from 2.18.18 to 2.19.36 molllyn1/node#48
Open

Uh oh!

doc: fix header escaping regression (Issue #22065)#22084

doc: fix header escaping regression (Issue #22065) #22084

Uh oh!

Conversation

rubys commented Aug 2, 2018• edited by refack Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

vsemozhetbytAug 2, 2018

Choose a reason for hiding this comment

Uh oh!

vsemozhetbytAug 2, 2018

Choose a reason for hiding this comment

Uh oh!

rubysAug 2, 2018

Choose a reason for hiding this comment

Uh oh!

vsemozhetbytAug 2, 2018

Choose a reason for hiding this comment

Uh oh!

vsemozhetbytAug 2, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt commented Aug 2, 2018

Uh oh!

vsemozhetbyt commented Aug 2, 2018

Uh oh!

rubys commented Aug 2, 2018

Uh oh!

This comment has been minimized.

Trott commented Aug 2, 2018

Uh oh!

richardlau commented Aug 3, 2018

Uh oh!

richardlauAug 3, 2018

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt commented Aug 3, 2018

Uh oh!

vsemozhetbyt commented Aug 3, 2018

Uh oh!

vsemozhetbyt commented Aug 3, 2018

Uh oh!

jasnell commented Aug 3, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rubys commented Aug 5, 2018

Uh oh!

jasnell commented Aug 12, 2018

Uh oh!

rubys commented Aug 12, 2018

Uh oh!

rubys commented Aug 13, 2018

Uh oh!

rubys commented Aug 13, 2018

Uh oh!

rubys commented Aug 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

rubys commented Aug 2, 2018•
edited by refack
Loading

vsemozhetbytAug 2, 2018•
edited
Loading

jasnell commented Aug 3, 2018•
edited
Loading