Skip to content
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

doc: header escaping regression #22065

Closed
rubys opened this issue Aug 1, 2018 · 4 comments · Fixed by #22140
Closed

doc: header escaping regression #22065

rubys opened this issue Aug 1, 2018 · 4 comments · Fixed by #22140
Labels
doc Issues and PRs related to the documentations. regression Issues related to regressions. tools Issues and PRs related to the tools directory.

Comments

@rubys
Copy link
Member

rubys commented Aug 1, 2018

Details here: #21936 (comment); pulled out as a separate issue so that we can discuss the changes and reference it in a pull request.

@rubys
Copy link
Member Author

rubys commented Aug 1, 2018

I'd prefer not to remove the escapes from the markdown. My thoughts are that the markdown should represent what an ideal markdown tool would consume, and our code needs to handle differences between our chosen tool (currently remark, et. al) and what that ideal is. I've provided a pull request to the code that underlies the remark tool, but it has yet to be evaluated: syntax-tree/mdast-util-to-hast#21

I believe that for now adding something like the following early in the unified processing of headers would address the needs for both HTML and JSON generation:

.replace(/\\./g, (match) => match[1])

I'll test that and provide a pull request. If/when the remark tool gets updated, we can look into removing this.

@vsemozhetbyt vsemozhetbyt added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. regression Issues related to regressions. labels Aug 1, 2018
@rubys
Copy link
Member Author

rubys commented Aug 1, 2018

Not related to the regression, but unless I take specific action to prevent it, a side effect of moving .use(html.preprocessText) earlier in the pipeline is that descriptions in the generated JSON will more closely match the descriptions in the generated HTML. An example to illustrate:

--- api0/all.json       2018-08-01 15:27:02.000000000 -0400
+++ api/all.json        2018-08-01 15:32:21.000000000 -0400
@@ -38,7 +38,7 @@
         {
           "textRaw": "Syscalls and man pages",
           "name": "syscalls_and_man_pages",
-          "desc": "<p>System calls like open(2) and read(2) define the interface between user programs\nand the underlying operating system. Node.js functions\nwhich simply wrap a syscall,\nlike <a href=\"fs.html#fs_fs_open_path_flags_mode_callback\"><code>fs.open()</code></a>, will document that. The docs link to the corresponding man\npages (short for manual pages) which describe how the syscalls work.</p>\n<p>Most Unix syscalls have Windows equivalents, but behavior may differ on Windows\nrelative to Linux and macOS. For an example of the subtle ways in which it's\nsometimes impossible to replace Unix syscall semantics on Windows, see <a href=\"https://github.com/nodejs/node/issues/4760\">Node\nissue 4760</a>.</p>",
+          "desc": "<p>System calls like <a href=\"http://man7.org/linux/man-pages/man2/open.2.html\"><code>open(2)</code></a> and <a href=\"http://man7.org/linux/man-pages/man2/read.2.html\"><code>read(2)</code></a> define the interface between user programs\nand the underlying operating system. Node.js functions\nwhich simply wrap a syscall,\nlike <a href=\"fs.html#fs_fs_open_path_flags_mode_callback\"><code>fs.open()</code></a>, will document that. The docs link to the corresponding man\npages (short for manual pages) which describe how the syscalls work.</p>\n<p>Most Unix syscalls have Windows equivalents, but behavior may differ on Windows\nrelative to Linux and macOS. For an example of the subtle ways in which it's\nsometimes impossible to replace Unix syscall semantics on Windows, see <a href=\"https://github.com/nodejs/node/issues/4760\">Node\nissue 4760</a>.</p>",
           "type": "misc",
           "displayName": "Syscalls and man pages"
         }

Note that where the JSON description had open(2) before, and <a href=\"http://man7.org/linux/man-pages/man2/open.2.html\"><code>open(2)</code></a> after.

I actually think it is a good thing for the JSON descriptions to more closely match the posted HTML, and will proceed under that assumption unless somebody objects. This is an example of the type of things I wanted to look at after the change was made and the first set of bugs were identified and resolved.

@rubys
Copy link
Member Author

rubys commented Aug 2, 2018

@vsemozhetbyt I'm not understanding what the issue with the generated JSON is:

                {
                  "textRaw": "urlSearchParams[Symbol.iterator]()",
                  "type": "method",
                  "name": "[Symbol.iterator]",
                  "signatures": [
                    {
                      "return": {
                        "textRaw": "Returns: {Iterator}",
                        "name": "return",
                        "type": "Iterator"
                      },
                      "params": []
                    }

What am I missing?

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Aug 2, 2018

I mean that now JSON is OK. But if I remove escapes from .md files and change RegExps in json.js, the JSON result is different and some fields are missing.

rubys added a commit to rubys/node that referenced this issue Aug 13, 2018
@rubys rubys closed this as completed in 59f8276 Aug 13, 2018
rvagg pushed a commit that referenced this issue Aug 15, 2018
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. regression Issues related to regressions. tools Issues and PRs related to the tools directory.
Projects
None yet
2 participants