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

@@ -198,7 +198,8 @@ function preprocessElements({ filename }) {
          heading.children = [{
            type: 'text',
            value: file.contents.slice(
-              position.start.offset, position.end.offset),
+              position.start.offset, position.end.offset)
+              .replace(/\\.{1}/g, (match) => match[1]),


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

@@ -198,7 +198,8 @@ function preprocessElements({ filename }) {
          heading.children = [{
            type: 'text',
            value: file.contents.slice(
-              position.start.offset, position.end.offset),
+              position.start.offset, position.end.offset)
+              .replace(/\\(.{1})/g, '$1')


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)

fixes nodejs#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: #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>

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

Landed in 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>

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

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

fixes nodejs#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 e956f6b Compare 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

rubys commented Aug 2, 2018 •

edited by refack

Loading

vsemozhetbyt Aug 2, 2018

vsemozhetbyt Aug 2, 2018

rubys Aug 2, 2018

vsemozhetbyt Aug 2, 2018

vsemozhetbyt Aug 2, 2018 •

edited

Loading

vsemozhetbyt commented Aug 2, 2018

vsemozhetbyt commented Aug 2, 2018

rubys commented Aug 2, 2018

This comment has been minimized.

Trott commented Aug 2, 2018

richardlau commented Aug 3, 2018

richardlau Aug 3, 2018

vsemozhetbyt commented Aug 3, 2018

vsemozhetbyt commented Aug 3, 2018

vsemozhetbyt commented Aug 3, 2018

jasnell commented Aug 3, 2018 •

edited

Loading

rubys commented Aug 5, 2018

jasnell commented Aug 12, 2018

rubys commented Aug 12, 2018

rubys commented Aug 13, 2018

rubys commented Aug 13, 2018

rubys commented Aug 13, 2018

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

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

Conversation

rubys commented Aug 2, 2018 • edited by refack Loading

Checklist

vsemozhetbyt Aug 2, 2018

Choose a reason for hiding this comment

vsemozhetbyt Aug 2, 2018

Choose a reason for hiding this comment

rubys Aug 2, 2018

Choose a reason for hiding this comment

vsemozhetbyt Aug 2, 2018

Choose a reason for hiding this comment

vsemozhetbyt Aug 2, 2018 • edited Loading

Choose a reason for hiding this comment

vsemozhetbyt commented Aug 2, 2018

vsemozhetbyt commented Aug 2, 2018

rubys commented Aug 2, 2018

This comment has been minimized.

Trott commented Aug 2, 2018

richardlau commented Aug 3, 2018

richardlau Aug 3, 2018

Choose a reason for hiding this comment

vsemozhetbyt commented Aug 3, 2018

vsemozhetbyt commented Aug 3, 2018

vsemozhetbyt commented Aug 3, 2018

jasnell commented Aug 3, 2018 • edited Loading

rubys commented Aug 5, 2018

jasnell commented Aug 12, 2018

rubys commented Aug 12, 2018

rubys commented Aug 13, 2018

rubys commented Aug 13, 2018

rubys commented Aug 13, 2018

rubys commented Aug 2, 2018 •

edited by refack

Loading

vsemozhetbyt Aug 2, 2018 •

edited

Loading

jasnell commented Aug 3, 2018 •

edited

Loading