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(filter): add after_route_render filter #4051

Merged
merged 3 commits into from Jan 5, 2020
Merged

Conversation

jiangtj
Copy link
Member

@jiangtj jiangtj commented Jan 4, 2020

What does it do?

#4048 See discussion here, add after_route_render implement it.

How to test

git clone -b filter https://github.com/jiangtj/hexo.git
cd hexo
npm install
npm test

Screenshots

Pull request tasks

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

@coveralls
Copy link

coveralls commented Jan 4, 2020

Coverage Status

Coverage increased (+0.0007%) to 97.424% when pulling 91c70ef on jiangtj:filter into 640b91e on hexojs:master.

test/scripts/hexo/hexo.js Outdated Show resolved Hide resolved
test/scripts/hexo/hexo.js Outdated Show resolved Hide resolved
test/scripts/hexo/hexo.js Outdated Show resolved Hide resolved
SukkaW
SukkaW previously approved these changes Jan 4, 2020
@SukkaW SukkaW requested a review from curbengh January 4, 2020 10:33
@jiangtj
Copy link
Member Author

jiangtj commented Jan 4, 2020

@SukkaW modified

BTW, Celebrating 1000 test cases. 😂

@curbengh
Copy link
Contributor

curbengh commented Jan 10, 2020

Or provide another filter to handle the HTML(after all rendered), after_render:html isn't very accurate, due to md to html or partial will trigger this filter. But expect to filter the final rendered HTML

So this filter only filter the final html file, right? In that case, after_route_render naming can be confusing, as route can refer to any file. I initially thought after_route_render pass every rendered html, js and css to the plugin, but actually only pass the html.

Wouldn't after_html_render (as initially suggested in #4048) be more accurate?

@SukkaW
Copy link
Member

SukkaW commented Jan 10, 2020

Wouldn't after_html_render (as initially suggested in #4048) be more accurate?

@curbengh But it will be confused with after_render:html then.

@curbengh
Copy link
Contributor

it will be confused with after_render:html then

They're functionally similar (to a plugin dev). We can make a note in the docs to recommend after_html_render over after_render:html, if the plugin dev doesn't mind backward compatibility.

@curbengh
Copy link
Contributor

Would it be possible to make after_render:html as an alias of after_html_render? If that's possible, this filter can be made as an internal API (e.g. _after_html_render), and then existing plugins can take advantage of it without changing filter.

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.

None yet

4 participants