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

refactor(external_link): utilize hexo-util isExternalLink() #3847

Closed
wants to merge 1 commit into from

Conversation

SukkaW
Copy link
Member

@SukkaW SukkaW commented Nov 9, 2019

What does it do?

This is a performance test of hexojs/hexo-util#124 to see if #3839 is still necessary. If newer isExternalLink() works, #3839 should be superseded by #3835.

How to test

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

Screenshots

Pull request tasks

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

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 97.118% when pulling c1037f0 on SukkaW:test-utilize-hexo-util into 6fa7ada on hexojs:master.

@coveralls
Copy link

coveralls commented Nov 9, 2019

Coverage Status

Coverage decreased (-0.03%) to 97.118% when pulling ebc42f0 on SukkaW:test-utilize-hexo-util into 6fa7ada on hexojs:master.

@SukkaW
Copy link
Member Author

SukkaW commented Nov 9, 2019

  • Node.js 10 27s
  • Node.js 12 30s
  • Node.js 12 28s

Emmmm, I think it is still necessary to have #3839 then.

@SukkaW SukkaW closed this Nov 9, 2019
@SukkaW SukkaW reopened this Nov 9, 2019
@curbengh
Copy link
Contributor

curbengh commented Nov 9, 2019

Emmmm, I think it is still necessary to have #3839 then.

#3839 is duplicate of hexojs/hexo-util#124, so #3835 is necessary, otherwise it defeats the purpose of hexojs/hexo-util#119.

@SukkaW
Copy link
Member Author

SukkaW commented Nov 9, 2019

@curbengh Yeah. So I need to find out what cause the difference between #3839 and hexojs/hexo-util#124.

@curbengh
Copy link
Contributor

curbengh commented Nov 9, 2019

My guess is that there is a possibility that hexo-util@1.5.0 is still used because it's installed by dependency as sub-dependency.

@SukkaW
Copy link
Member Author

SukkaW commented Nov 9, 2019

@curbengh Currently, the only difference between #3839 and hexojs/hexo-util#124 is how it handle exclude. #3839 converts exclude into array first, then isExternal() uses exclude as an array directly. While hexojs/hexo-util#124 still converts exclude into array every time isExternalLink() is called.


But according to #3839 (comment), it shouldn't cause so much difference:

After 6d9ed7a , it drops from 76s to 72s on Node.js 13 (and Node.js 12 remains 76s).


Bingo! I have triggered a rebuild for #3839, and the performance drops to more or less the same with this PR now.

@SukkaW SukkaW marked this pull request as ready for review November 9, 2019 08:52
@SukkaW
Copy link
Member Author

SukkaW commented Nov 9, 2019

Rebased and ready for review. Should be merged after a newer version of hexo-util is released.

@SukkaW SukkaW requested a review from curbengh November 9, 2019 12:23
@SukkaW
Copy link
Member Author

SukkaW commented Nov 9, 2019

I think I might add extra changes to this PR.


It is possible to only register external_link filter only when enabled in external_link related config, but what should we do to test this?

@curbengh curbengh added this to the v4.1.0 milestone Nov 18, 2019
@SukkaW SukkaW changed the title test(external_link): utilize hexo-util isExternalLink() refactor(external_link): utilize hexo-util isExternalLink() Nov 21, 2019
@curbengh
Copy link
Contributor

Please update thie repo's hexo-util to 1.6.0.

@curbengh
Copy link
Contributor

Need to include 8ecb782 when updating hexo-util to 1.6.0 to pass the test.

@SukkaW SukkaW mentioned this pull request Nov 28, 2019
2 tasks
@SukkaW
Copy link
Member Author

SukkaW commented Nov 28, 2019

Should be superseded by #3888

@SukkaW SukkaW closed this Nov 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants