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

fix: external_link should use after_render #3675

Merged
merged 9 commits into from Oct 1, 2019

Conversation

@curbengh
Copy link
Contributor

@curbengh curbengh commented Aug 16, 2019

What does it do?

after_post_render only applies to rendered posts/pages, so external_link doesn't work on links specified in themes. This PR fixes that by using after_render:html instead.

Added options

external_link:
  enable: true
  field: 'site'|'post'
  exclude:
    - 'exclude1.com'
    - 'exclude2.com'

Backward compatible with

external_link: true|false

How to test

git clone -b external-link 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.
@coveralls
Copy link

@coveralls coveralls commented Aug 16, 2019

Coverage Status

Coverage increased (+0.03%) to 97.281% when pulling 26b010a on curbengh:external-link into ffe4eaa on hexojs:master.

@curbengh
Copy link
Contributor Author

@curbengh curbengh commented Aug 16, 2019

not sure why it fails the test (including local unit test). It works in actual use case.

@SukkaW
Copy link
Member

@SukkaW SukkaW commented Aug 16, 2019

I don't see why external_link should be registered at after_render.

If there are external links that not in the post.content, which means those links is in the theme layouts, then _target attributes can be added directly in the theme template.

@curbengh
Copy link
Contributor Author

@curbengh curbengh commented Aug 16, 2019

If there are external links that not in the post.content, which means those links is in the theme layouts, then _target attributes can be added directly in the theme template.

I assume user expects external_link to apply to all the links including those listed in a theme.

We can't expect user to verify the rendered html and modify the theme accordingly.

@SukkaW
Copy link
Member

@SukkaW SukkaW commented Aug 16, 2019

We can't expect user to verify the rendered html and modify the theme accordingly.

For me, best practice is to let user decide which to choose, but adding a configuration might cause a break change and have no backward compatibility.

@curbengh
Copy link
Contributor Author

@curbengh curbengh commented Aug 16, 2019

For me, best practice is to let user decide which to choose...

you mean choosing which links to apply?
before this PR, if user does not disable external_link, all the links in posts/pages will be applied. This PR simply include links in the theme as well, which I don't think is a breaking change; it means the links in the theme will behave the same as posts/pages'.

I understand this approach incur performance cost, but this is necessary for consistency.


What I meant in previous comment is that we can't expect to verify each and every <a> in the rendered html, then cross-check with all the links in theme. Links in posts/pages should behave the same as theme's.

@curbengh
Copy link
Contributor Author

@curbengh curbengh commented Aug 17, 2019

On the user customization, we can introduce an extra option (e.g. exclude_external_link) to exclude user-specified domains from being processed by external_link. It's relatively easy to implement, just need to port over these lines.

@SukkaW
Copy link
Member

@SukkaW SukkaW commented Aug 17, 2019

IMHO, since we are going to release a big version 4.0.0, we could just bring something new to external_link like:

external_link:
  enable: true
  onlypost: false
  exclude:
    - 'exclude1.com'
    - 'exclude2.com'

And still accept external_link: true to maintain backwards compatibility.

@curbengh
Copy link
Contributor Author

@curbengh curbengh commented Aug 18, 2019

I can introduce exclude_external_link (or similar) in a separate PR.

@curbengh
Copy link
Contributor Author

@curbengh curbengh commented Aug 23, 2019

rebased

@curbengh
Copy link
Contributor Author

@curbengh curbengh commented Aug 28, 2019

rebased

@curbengh
Copy link
Contributor Author

@curbengh curbengh commented Sep 25, 2019

IMHO, since we are going to release a big version 4.0.0, we could just bring something new to external_link like:

external_link:
  enable: true
  onlypost: false
  exclude:
    - 'exclude1.com'
    - 'exclude2.com'

And still accept external_link: true to maintain backwards compatibility.

Added in latest commits.

SukkaW
SukkaW previously approved these changes Sep 25, 2019
@curbengh
Copy link
Contributor Author

@curbengh curbengh commented Sep 26, 2019

btw, I did attempt to dedup based on approach used in hexo-filter-nofollow, but somehow it affects other unit tests.

@curbengh curbengh added this to the v4.0.0 milestone Sep 26, 2019
@curbengh curbengh added this to To do in v4.0.0 Sep 26, 2019
@curbengh curbengh requested a review from SukkaW Sep 27, 2019
@curbengh
Copy link
Contributor Author

@curbengh curbengh commented Sep 27, 2019

Rebased to fix failing test.

v4.0.0 automation moved this from To do to In progress Sep 30, 2019
SukkaW
SukkaW approved these changes Sep 30, 2019
@curbengh curbengh merged commit 6142b84 into hexojs:master Oct 1, 2019
3 of 4 checks passed
v4.0.0 automation moved this from In progress to Done Oct 1, 2019
@curbengh curbengh deleted the external-link branch Oct 1, 2019
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
yy4r added 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v4.0.0
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants