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

refactor(toc_helper): utilize hexo-util #3850

Merged
merged 3 commits into from
Dec 20, 2019
Merged

refactor(toc_helper): utilize hexo-util #3850

merged 3 commits into from
Dec 20, 2019

Conversation

SukkaW
Copy link
Member

@SukkaW SukkaW commented Nov 9, 2019 •
edited
Loading

What does it do?

This PR is the last pieces of #3677. We finally made it.

How to test

git clone -b htmlparser2-toc https://github.com/sukkaw/hexo.git
cd hexo
npm install
npm test

Screenshots

Since hexo-theme-landscape is not using toc helper, I have set up a benchmark using my hexo-theme-suka, with hexo's meta_generator and external_link disabled.

Node.js 8

Process Time Memory
Current Master Branch 21s 528MB
This PR 17s 421MB

Nodejs.10

Process Time Memory
Current Master Branch 20s 527MB
This PR 17s 464MB

Nodejs.12

Process Time Memory
Current Master Branch 24s 460MB
This PR 22s 470MB

Pull request tasks

  • Add test cases for the changes.
  • Passed the CI test.

@SukkaW SukkaW requested review from curbengh and a team November 9, 2019 19:38
@coveralls
Copy link

Coverage Status

Coverage increased (+0.003%) to 97.147% when pulling 7a4f797 on SukkaW:htmlparser2-toc into 6fa7ada on hexojs:master.

@coveralls
Copy link

coveralls commented Nov 9, 2019 •
edited
Loading

Coverage Status

Coverage decreased (-0.006%) to 97.105% when pulling df7ebcb on SukkaW:htmlparser2-toc into 9b49688 on hexojs:master.

@curbengh curbengh mentioned this pull request Nov 9, 2019
Closed
4 tasks
@stevenjoezhang stevenjoezhang mentioned this pull request Nov 11, 2019
Closed
33 tasks
tomap
tomap previously approved these changes Nov 11, 2019
View reviewed changes
@SukkaW
Copy link
Member Author

SukkaW commented Nov 17, 2019

After this PR is merged, I might begin to address #3288.

@SukkaW
Copy link
Member Author

SukkaW commented Nov 18, 2019

This PR should be superseded by utilizing newer version of hexo-util after hexojs/hexo-util#137 is merged and released in [email protected].

@stevenjoezhang stevenjoezhang mentioned this pull request Dec 3, 2019
Merged
@curbengh
Copy link
Contributor

curbengh commented Dec 7, 2019

This PR should be superseded by utilizing newer version of hexo-util after hexojs/hexo-util#137.

Can this PR use hexojs/hexo-util#137 branch temporarily for testing? Once that PR is released, just need to update hexo-util in this PR.

@curbengh curbengh changed the title refactor(toc_helper): relpace cheerio with htmlparser2 refactor(toc_helper): replace cheerio with htmlparser2 Dec 10, 2019
@curbengh curbengh added this to the 4.2.0 milestone Dec 11, 2019
@curbengh curbengh mentioned this pull request Dec 12, 2019
Closed
2 tasks
curbengh
curbengh reviewed Dec 13, 2019
View reviewed changes
lib/plugins/helper/toc.js
max_depth: 6,
class: 'toc',
list_number: true
}, options);
Copy link
Contributor

@curbengh curbengh Dec 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you plan to add min_depth option in this PR? Or in a separate PR (due to addition of unit test)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@curbengh No. The current unit test for toc helper is completely a mess. I might refactor the unit test when bringing up the min_depth.

curbengh
curbengh previously approved these changes Dec 18, 2019
View reviewed changes
Copy link
Contributor

@curbengh curbengh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally. Pending release of hexojs/hexo-util#137.

@curbengh curbengh mentioned this pull request Dec 18, 2019
Merged
@SukkaW SukkaW mentioned this pull request Dec 19, 2019
Merged
2 tasks
@curbengh curbengh removed this from the 4.2.0 milestone Dec 19, 2019
@curbengh
Copy link
Contributor

curbengh commented Dec 19, 2019 •
edited
Loading

So far we haven't merge any new feature yet (except for #3963), perhaps this can be released into 4.1.2?

@SukkaW
Copy link
Member Author

SukkaW commented Dec 20, 2019

@curbengh min_depth could be bring up along with tocObj() so there won't be only one feature.

@SukkaW SukkaW requested a review from curbengh December 20, 2019 01:47
@SukkaW
Copy link
Member Author

SukkaW commented Dec 20, 2019 •
edited
Loading

@curbengh Rebased. It is now ready for review.

@SukkaW SukkaW changed the title refactor(toc_helper): replace cheerio with htmlparser2 refactor(toc_helper): utilize hexo-util Dec 20, 2019
@curbengh
Copy link
Contributor

min_depth could be bring up along with tocObj() so there won't be only one feature.

That makes two features then. Sufficient for 4.2.0. #3891 can be deferred to 4.3+.

@SukkaW SukkaW merged commit a381dda into hexojs:master Dec 20, 2019
@SukkaW SukkaW mentioned this pull request Dec 21, 2019
Merged
1 task
thom4parisot pushed a commit to thom4parisot/hexo that referenced this pull request Jan 17, 2020
@SukkaW
refactor(toc_helper): utilize hexo-util (hexojs#3850)
a8efcdf 
* refactor(toc_helper): relpace cheerio with htmlparser2

* test(toc_helper): use classic for loop

* refactor(toc_helper): utilize tocObj
@snyk-bot snyk-bot mentioned this pull request Mar 16, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@curbengh curbengh curbengh approved these changes

@tomap tomap tomap left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@SukkaW @coveralls @curbengh @tomap