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.

Sign up for GitHub

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

Jump to bottom

tools: update doc generator dependencies #40042

Closed
wants to merge 1 commit into from
Closed

tools: update doc generator dependencies #40042

wants to merge 1 commit into from

Conversation

targos
Copy link
Member

@targos targos commented Sep 8, 2021

This is not working yet. There are failing doctool tests and the generated files have issues.

/cc @aduh95 @Trott

@targos targos added help wanted Issues that need assistance from volunteers or PRs that need help to proceed. wip Issues and PRs that are still a work in progress. doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels Sep 8, 2021
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Sep 8, 2021
@targos targos mentioned this pull request Sep 8, 2021
Closed
@targos
Copy link
Member Author

targos commented Sep 8, 2021 •
edited
Loading

Note that just updating remark-html like in #40033 doesn't help. The breaking change is exactly from remark-html 13.0.2.

@targos
Copy link
Member Author

targos commented Sep 8, 2021

I just read the changelog: https://github.com/remarkjs/remark-html/releases/tag/13.0.2
It looks like we rely on the "unsafe" behavior.

@targos
Copy link
Member Author

targos commented Sep 8, 2021

I opened #40043 to silence the dependabot alert while we work on this.

@aduh95
Copy link
Contributor

aduh95 commented Sep 8, 2021

generated files have issues.

Has this been resolved by #40043 or do you still need help? I've pulled the branch on my local repo and didn't find any obvious issue with the generated HTML.

@targos
Copy link
Member Author

targos commented Sep 9, 2021 •
edited
Loading

No issues anymore, but many differences with the current version. I'd appreciate help to review these and compare the rendered pages visually

@Trott
Copy link
Member

Trott commented Sep 17, 2021

The .json files are different and I don't know enough about how they're used to be sure from a glance that it matters. I'll try to dig in, but if someone else does it before I do, I will not complain.

@Trott
Copy link
Member

Trott commented Sep 17, 2021

Looks to me like this is correct and the current version gets it wrong.

@Trott
Copy link
Member

Trott commented Sep 17, 2021

Before this PR, it adds in wrong <p> blocks in places like this:

image

With this PR, it's all one block:

image

@Trott Trott added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed help wanted Issues that need assistance from volunteers or PRs that need help to proceed. labels Sep 17, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 17, 2021
@nodejs-github-bot
Copy link
Collaborator

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

@Trott Trott mentioned this pull request Sep 17, 2021
Closed
@Trott

This comment has been minimized.

Sign in to view
@Trott Trott requested a review from aduh95 September 17, 2021 13:49
@Trott

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Sep 18, 2021
Open
17 tasks
@targos targos removed the wip Issues and PRs that are still a work in progress. label Sep 18, 2021
targos added a commit that referenced this pull request Sep 18, 2021
@targos
tools: update doc generator dependencies
2b080cb 
PR-URL: #40042
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@targos
Copy link
Member Author

targos commented Sep 18, 2021

Landed in 2b080cb

@targos targos closed this Sep 18, 2021
@targos targos deleted the tools-doc branch September 18, 2021 14:33
@targos targos mentioned this pull request Sep 18, 2021
Closed
This was referenced Sep 19, 2021
Open
Open
BethGriggs pushed a commit that referenced this pull request Sep 21, 2021
@targos @BethGriggs
tools: update doc generator dependencies
c2fefb0 
PR-URL: #40042
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
BethGriggs pushed a commit that referenced this pull request Sep 21, 2021
@targos @BethGriggs
tools: update doc generator dependencies
a8c99d9 
PR-URL: #40042
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Sep 21, 2021
Merged
1 task
This was referenced Sep 22, 2021
Open
Open
Open
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95 aduh95 approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations. needs-ci PRs that need a full CI run. 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.
5 participants
@targos @aduh95 @Trott @nodejs-github-bot @jasnell