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(isExternal): avoid undefined & improve performance #124

Merged
merged 6 commits into from
Nov 17, 2019

Conversation

SukkaW
Copy link
Member

@SukkaW SukkaW commented Nov 8, 2019

The PR fixes #123 with a simpler way. This PR also includes the changes from hexojs/hexo#3839 , which is a poc of performance improvements.

@SukkaW SukkaW requested a review from curbengh November 8, 2019 13:10
@coveralls
Copy link

coveralls commented Nov 8, 2019

Coverage Status

Coverage decreased (-0.09%) to 95.364% when pulling 6ea2fb7 on SukkaW:perf-is-external into 71bbcc4 on hexojs:master.

const host = data.hostname;
const sitehost = typeof urlObj(config.url) === 'object' ? urlObj(config.url).hostname : config.url;
const sitehost = parse(config.url).hostname || config.url;
// Pass a base to new URL
Copy link
Contributor

@curbengh curbengh Nov 9, 2019

Choose a reason for hiding this comment

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

I would say the purpose is // Relative url handling.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated to // handle relative url since there is a handle mailto: javascript: vbscript: and so on below.

const sitehost = typeof urlObj(config.url) === 'object' ? urlObj(config.url).hostname : config.url;
const sitehost = parse(config.url).hostname || config.url;
// Pass a base to new URL
const data = new URL(url, `http://${sitehost}`);

if (!sitehost || typeof data === 'string') return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

with L15, typeof data is no longer required since it will always be an "object".

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed.

@@ -18,17 +10,19 @@ const urlObj = (str) => {

function isExternalLink(url) {
Copy link
Contributor

Choose a reason for hiding this comment

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

suggest to rename to path or input to not confuse with new URL().

Copy link
Member Author

Choose a reason for hiding this comment

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

input might be a good idea.

@curbengh
Copy link
Contributor

curbengh commented Nov 9, 2019

It's fine to use isExternalLink(url) in documentation.

curbengh
curbengh previously approved these changes Nov 9, 2019
@curbengh curbengh dismissed their stale review November 9, 2019 04:54

handling null config.url

@SukkaW
Copy link
Member Author

SukkaW commented Nov 9, 2019

@curbengh So should we should throw a TypeError if config.url is null or undefined?

@SukkaW SukkaW requested a review from curbengh November 13, 2019 07:32
@curbengh curbengh merged commit e2e3739 into hexojs:master Nov 17, 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.

3 participants