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

Leave IDN intact in permalink #116

Merged
merged 4 commits into from
Nov 3, 2019
Merged

Leave IDN intact in permalink #116

merged 4 commits into from
Nov 3, 2019

Conversation

tomap
Copy link
Contributor

@tomap tomap commented Oct 27, 2019

Because IDN only contain unicode, they do not need to be punycode encoded.
(I bought one, I want the nice version, not the ugly one)
See https://nodejs.org/docs/latest/api/url.html#url_url_format_url_options

@coveralls
Copy link

coveralls commented Oct 27, 2019

Coverage Status

Coverage increased (+0.007%) to 95.432% when pulling e14f8e0 on tomap:IDNUnicode into 4f07d2b on hexojs:master.

@SukkaW SukkaW requested a review from curbengh October 27, 2019 14:11
@SukkaW
Copy link
Member

SukkaW commented Oct 27, 2019

Maybe we should implement two encode_url() method for different usages.

@tomap
Copy link
Contributor Author

tomap commented Oct 27, 2019

I'm not sure there is a usage where IDN needs to be punycode encoded.

@@ -24,7 +24,8 @@ const encodeURL = (str) => {
if (parsed.origin === 'null') return str;

parsed.search = encodeURI(safeDecodeURI(parsed.search));
return parsed.toString();
// preserve host (international domain name) as is.
return format(parsed, { unicode: true});
Copy link
Contributor

@curbengh curbengh Oct 28, 2019

Choose a reason for hiding this comment

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

url.format() doesn't support port number in Node 8, see #114. This explains the failed test in Node 8.

Copy link
Contributor

@curbengh curbengh Oct 28, 2019

Choose a reason for hiding this comment

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

A workaround is to use punycode api

const { toUnicode } = require('./punycode');
return parsed.toString().replace(parsed.hostname, toUnicode(parsed.hostname));

Copy link
Contributor

Choose a reason for hiding this comment

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

readme also needs to be updated.

Copy link
Contributor Author

@tomap tomap Oct 28, 2019

Choose a reason for hiding this comment

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

node 8 EOL is arround the corner, and will probably imply that this package, as well as many other and also hexo have a major version bump.
So this PR can wait a bit for another PR to drop node 8 compatibility

Copy link
Contributor

@curbengh curbengh Nov 2, 2019

Choose a reason for hiding this comment

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

we can use the workaround temporarily until node 8 EOL. I don't think this is a breaking change, I do agree IDN users would prefer not to punycode.

If this PR wait for hexo's major version bump, it would be many months away.

Copy link
Contributor

Choose a reason for hiding this comment

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

Added the workaround. another change introduced by this PR is that IDN whether it's in unicode or punycoded is always encoded to unicode.

encodeURL(http://xn--br-mia.com/baz')
// http://bár.com/baz

@curbengh
Copy link
Contributor

curbengh commented Oct 28, 2019

I'm not sure there is a usage where IDN needs to be punycode encoded.

When permalink is encoded by default, it breaks encodeURI() (permalink is encoded twice).

If a plugin wants to remain compatible with <v4, they can use encodeURI(decodeURI()) or encodeURL() instead.

If the IDN is not punycoded, encodeURI(decodeURI()) will percent-encode the domain.

But of course, as we learned from hexo-generator-feed & hexo-generator-sitemap, encodeURI()/encodeURI(decodeURI()) should be avoided and use encodeURL() instead.

Note that I still +1 this PR.

curbengh
curbengh previously approved these changes Nov 2, 2019
@curbengh curbengh requested a review from a team November 2, 2019 07:52
SukkaW
SukkaW previously approved these changes Nov 2, 2019
@curbengh curbengh dismissed stale reviews from SukkaW and themself via e14f8e0 November 2, 2019 10:38
@curbengh curbengh requested a review from SukkaW November 2, 2019 10:40
@curbengh curbengh mentioned this pull request Nov 2, 2019
@curbengh curbengh merged commit 8ebe0eb into hexojs:master Nov 3, 2019
@curbengh curbengh added this to In progress in Drop legacy URL API via automation Nov 5, 2019
@SukkaW SukkaW moved this from In progress to Done in Drop legacy URL API Nov 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants