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

feat(permalink): add pretty_urls option to remove index.html from url #3691

Merged
merged 6 commits into from Sep 18, 2019

Conversation

curbengh
Copy link
Contributor

@curbengh curbengh commented Aug 28, 2019

What does it do?

Currently permalink variable of post, page, tag, category may end with "index.html". This PR adds canonical_url option to remove that. canonical_url defaults to false to avoid breaking change.

This is useful for SEO when web server (including hexo server) rewrite "index.html", so the url without it is the canonical version.

This way, plugins that use post.permalink don't need to remove it. Tested with hexo-generator-sitemap.
Related:

Note this PR doesn't cover this.url(#3661).

New option:

pretty_urls:
  trailing_index: true

How to test

git clone -b canonical https://github.com/curbengh/hexo.git
cd hexo
npm install
npm test

Pull request tasks

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

Fixes #1306 cc @sapegin @pyyzcwg2833

@coveralls
Copy link

@coveralls coveralls commented Aug 28, 2019

Coverage Status

Coverage increased (+0.08%) to 97.23% when pulling 8c0ca00 on curbengh:canonical into 01eebda on hexojs:master.

@SukkaW
Copy link
Member

@SukkaW SukkaW commented Aug 28, 2019

canonical_url is not accurate enough. IMHO, url_strip is more appropriate.

@curbengh
Copy link
Contributor Author

@curbengh curbengh commented Aug 28, 2019

I opt for canonical_url as similar function (named canonical) can be found in hexo-theme-next.

@SukkaW
Copy link
Member

@SukkaW SukkaW commented Aug 28, 2019

@curbengh

The canonical config in NexT theme is used to control whether add link[canonical] tag to <head>.
Since this PR introduced a feature to remove index.html in url, IMHO the option should be named something strip.

@yoshinorin
Copy link
Member

@yoshinorin yoshinorin commented Aug 28, 2019

canonical_url is not accurate enough.

+1

Maybe we can find more better naming. url_ending, url_trailing etc... also url_strip


And I would like to implement a trailing slash option.

_config.yml

# following namings are an example.
url_trailing:
  include_html: true      # https://example.com/foo/index.html
  slash: true             # This option will be enabled if include_html is false. https://example.com/foo/

If we implement this idea user can select which they use from three patterns url.

https://example.com/foo/index.html
https://example.com/foo
https://example.com/foo/

But, I'm worry followings if implement trailing slash option.

  1. Increase source code complexity
  2. User may confusing

How do you think?

@curbengh
Copy link
Contributor Author

@curbengh curbengh commented Aug 29, 2019

The canonical config in NexT theme is used to control whether add link[canonical] tag to <head>.

I'm aware of that, what I meant is the end goal is the same, which is to tell the links to search engines or readers that the links without "index.html" is the canonical version.

I dismissed the idea url_strip: false/true because it doesn't explain the goal and is more confusing.

But I do like the suggestion by @yoshinorin

trailing_url:
  include_html: true
  trailing_slash: true

And I would like to implement a trailing slash option.

Sounds good.

@curbengh
Copy link
Contributor Author

@curbengh curbengh commented Aug 30, 2019

I changed the option to:

trailing_url:
  trailing_index: true

@SukkaW
Copy link
Member

@SukkaW SukkaW commented Aug 30, 2019

LGTM!

@curbengh
Copy link
Contributor Author

@curbengh curbengh commented Aug 30, 2019

Thanks. Anyway, this can defer to v4.1.

@curbengh
Copy link
Contributor Author

@curbengh curbengh commented Sep 1, 2019

just stumbled upon Cryogen which uses option,

clean-urls: 'trailing-slash'

@curbengh curbengh changed the title feat(permalink): add canonical_url option to remove index.html from url feat(permalink): add trailing_index option to remove index.html from url Sep 2, 2019
@yoshinorin
Copy link
Member

@yoshinorin yoshinorin commented Sep 14, 2019

@curbengh

Sorry so late reply.
IMHO trailing_url.trailing_index is redundant naming.


just stumbled upon Cryogen which uses option,

Is this mean to specify the option by a string? If yes, it seems a better way.

@curbengh
Copy link
Contributor Author

@curbengh curbengh commented Sep 14, 2019

Specify by string means only one option though. handling multiple options in an array can be awkward.

What about trailing_url.html?

@SukkaW
Copy link
Member

@SukkaW SukkaW commented Sep 14, 2019

@curbengh @yoshinorin

From Netlify dashboard there is an option: Assets Optimization - Pretty URLs:

image

We might name the config like:

{
  "pretty_urls": {
    "trailing_index": false,
    "trailing_html": false,
    "trailing_slash": true
  }
}

@yoshinorin
Copy link
Member

@yoshinorin yoshinorin commented Sep 16, 2019

@SukkaW It seems better idea :)
@curbengh If you have no objections would you please update like Netlify options?

@curbengh
Copy link
Contributor Author

@curbengh curbengh commented Sep 17, 2019

Done 👍

Copy link
Member

@yoshinorin yoshinorin left a comment

LGTM 🚀

@yoshinorin yoshinorin changed the title feat(permalink): add trailing_index option to remove index.html from url feat(permalink): add pretty_urls option to remove index.html from url Sep 17, 2019
SukkaW
SukkaW approved these changes Sep 17, 2019
@curbengh curbengh merged commit ab90487 into hexojs:master Sep 18, 2019
3 of 4 checks passed
@SukkaW
Copy link
Member

@SukkaW SukkaW commented Sep 18, 2019

Also, should we bring up trailing_html (When disabled, /about.html => /about) and trailing_slash (When disabled, /about/ => /about) options in 4.0.0?

@yoshinorin
Copy link
Member

@yoshinorin yoshinorin commented Sep 18, 2019

Yes, if possible.
I'm going to implement it but now I'm a bit busy... if someone creat PR I will review it.

@SukkaW
Copy link
Member

@SukkaW SukkaW commented Sep 18, 2019

@yoshinorin I might implement it after #3708 & #3710 merged.

@NoahDragon NoahDragon mentioned this pull request Sep 28, 2019
53 tasks
curbengh added a commit to curbengh/hexo-starter that referenced this issue Oct 11, 2019
- pretty_urls.trailing_index: hexojs/hexo#3691
- external_link.enable: hexojs/hexo#3675
- meta_generator: hexojs/hexo#3653
- use_date_for_updated: hexojs/hexo#3235
@curbengh curbengh deleted the canonical branch Dec 13, 2019
yy4r pushed a commit to yy4r/blogback that referenced this issue Jan 16, 2020
- pretty_urls.trailing_index: hexojs/hexo#3691
- external_link.enable: hexojs/hexo#3675
- meta_generator: hexojs/hexo#3653
- use_date_for_updated: hexojs/hexo#3235
curbengh pushed a commit to curbengh/curbengh.github.io that referenced this issue Apr 15, 2020
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.

4 participants