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

test/refactor: update isExternalLink #3839

Closed
wants to merge 4 commits into from

Conversation

SukkaW
Copy link
Member

@SukkaW SukkaW commented Nov 6, 2019

What does it do?

Try to fix #3833

#3833 (comment)

Screenshots

Pull request tasks

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

@SukkaW SukkaW requested a review from curbengh November 6, 2019 05:00
@coveralls
Copy link

coveralls commented Nov 6, 2019

Coverage Status

Coverage increased (+0.004%) to 97.148% when pulling 20d4ae4 on SukkaW:test_external_link_perf into a088ede on hexojs:master.

@SukkaW SukkaW changed the title refactor: update isExternalLink test/refactor: update isExternalLink Nov 6, 2019
@SukkaW
Copy link
Member Author

SukkaW commented Nov 6, 2019

After 60acb95 , I managed to lower the generation duration from 86s to 76s on Node.js 12 & 13.

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

@SukkaW
Copy link
Member Author

SukkaW commented Nov 6, 2019

From jsdelivr/bootstrapcdn#1391 , the matainer of bootstrapcdn said the url.parse is faster. Maybe we should have a benchmark on which one is faster.

@SukkaW
Copy link
Member Author

SukkaW commented Nov 6, 2019

According to the benchmark, the current "safe whatwg url" or urlObj is 2x slower than pure new URL, 6x slower than url.parse().

Since whatwg url is required in isExternalLink to ignore mailto: and so on, I might pass sitehost as base in new URL to avoid urlObj.

@SukkaW
Copy link
Member Author

SukkaW commented Nov 6, 2019

The generation duration is dropped to:

  • Node.js 10: 27s
  • Node.js 12: 26s
  • Node.js 13: 24s

@SukkaW SukkaW marked this pull request as ready for review November 6, 2019 14:02
@SukkaW
Copy link
Member Author

SukkaW commented Nov 6, 2019

@curbengh

It is now ready for review.

After this PR is approved, we should close this PR and update isExternalLink in hexo-util.

Although url.parse() is deprecated, since it is only deprecated in documents, and it is much faster and cleaner than whatwg url, we should use it when necessary. #3812 uses whatwg url only for detect mailto: things.

@curbengh
Copy link
Contributor

Superseded by #3847.

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

Successfully merging this pull request may close these issues.

Regression in CI benchmark
3 participants