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(nunjucks): dedicated nunjucks renderer #4356

Merged
merged 8 commits into from
Jul 7, 2020

Conversation

SukkaW
Copy link
Member

@SukkaW SukkaW commented Jun 13, 2020

What does it do?

The continuation of #2903.

In every case, hexo-renderer-* is considered as a plugin of hexo (not a dependency of hexo). When hexo uses swig as a default renderer, hexo-swig-renderer exists but it is not a dependency of hexo.

In this PR I remove hexo-renderer-nunjucks and bring up a dedicated nunjucks renderer.

I have written a custom toArray to avoid lodash.toarray,

How to test

git clone -b split-nunjucks https://github.com/sukkaw/hexo.git
cd hexo
npm install
npm test

Screenshots

Pull request tasks

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

@SukkaW SukkaW requested review from thom4parisot and a team June 13, 2020 16:16
@SukkaW SukkaW force-pushed the split-nunjucks branch 2 times, most recently from 5e6f891 to fd71d31 Compare June 13, 2020 16:40
@jiangtj
Copy link
Member

jiangtj commented Jun 15, 2020

I tested the forked next theme from mimi, but got error. Is there any difference between next built-in and this? @stevenjoezhang

# create project
hexo init test-dir
cd test-dir
# fork and use next theme
git clone https://github.com/next-theme/hexo-theme-next themes/next
rm themes/next/scripts/renderer.js
hexo config theme next
# use this hexo version
yarn add sukkaw/hexo#split-nunjucks
# run
hexo cl && hexo s

image

@SukkaW
Copy link
Member Author

SukkaW commented Jun 15, 2020

@jiangtj

It appears that there are some nunjucks filters added by @stevenjoezhang in next-theme (For example json filter).

Also, the PR (migrated from @oncletom) uses fs.readFileSync, while @stevenjoezhang let nunjucks resolves the template file path.

@SukkaW
Copy link
Member Author

SukkaW commented Jun 15, 2020

@stevenjoezhang I could include json filter in the PR as well if you wish.

@stevenjoezhang
Copy link
Member

I would appreciate if the json filter could be included. The Nunjucks build-in filter dump does not handle undefined value.
https://mozilla.github.io/nunjucks/templating.html#dump

@SukkaW
Copy link
Member Author

SukkaW commented Jun 15, 2020

@stevenjoezhang A new filter safeDump has been added and test cases are introduced.

@coveralls
Copy link

coveralls commented Jun 15, 2020

Coverage Status

Coverage remained the same at 97.742% when pulling 821e075 on SukkaW:split-nunjucks into 600b43a on hexojs:master.

@SukkaW SukkaW added this to the 5.0.0 milestone Jun 16, 2020
@curbengh
Copy link
Contributor

I tested the forked next theme from mimi, but got error. Is there any difference between next built-in and this?
image

Tested with latest PR update, same error.

@SukkaW
Copy link
Member Author

SukkaW commented Jun 22, 2020

@curbengh I have ported the nunjucks renderer from @stevenjoezhang. This time the nunjucks renderer should work.

curbengh
curbengh previously approved these changes Jun 26, 2020
Copy link
Contributor

@curbengh curbengh left a comment

Choose a reason for hiding this comment

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

WFM, this time I didn't remove themes/next/scripts/renderer.js.

@SukkaW
Copy link
Member Author

SukkaW commented Jun 26, 2020

this time I didn't remove themes/next/scripts/renderer.js.

@curbengh Remove it and try again see if it works?

@curbengh
Copy link
Contributor

Is it supposed to get removed? I got following error after remove:

ERROR Template render error: (themes/next/layout/page.njk)
  Error: template not found: _layout.njk

@SukkaW
Copy link
Member Author

SukkaW commented Jun 28, 2020

Is it supposed to get removed? I got following error after remove:

Yes. New NexT theme has a built-in nunjucks renderer which should be removed after this PR is merged (As the PR is attempt to replace it).

There must be something wrong with relative path resolving.
Help wanted. @stevenjoezhang

Copy link
Member

@thom4parisot thom4parisot left a comment

Choose a reason for hiding this comment

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

I do not understand the difference between "dedicated renderer" and "a plugin of hexo" (as a user feature).

The initial intent of providing the renderer as a plugin was to uncouple the rendering with hexo mecanics — this way, defaulting to a different renderer is just a matter of switching a renderer dependency from one to another.

What you are doing here is fairly good addition to the core plugin, it would totally make sense you contribute it upstream.

stevenjoezhang
stevenjoezhang previously approved these changes Jul 2, 2020
@SukkaW
Copy link
Member Author

SukkaW commented Jul 2, 2020

Rebased.

@stevenjoezhang Would you mind reviewing the PR again?
@oncletom Would you mind adding your review?

@curbengh Try removing next-theme's nunjucks.js and try this PR again? My local dummy blog shows fine.

@curbengh
Copy link
Contributor

curbengh commented Jul 3, 2020

You mean themes/next/scripts/renderer.js? Same error even with the latest commit (of this PR).

Error: template not found: _layout.njk

stevenjoezhang
stevenjoezhang previously approved these changes Jul 3, 2020
@SukkaW
Copy link
Member Author

SukkaW commented Jul 3, 2020

@curbengh This time should work.

@stevenjoezhang stevenjoezhang self-requested a review July 4, 2020 00:55
Copy link
Member

@jiangtj jiangtj left a comment

Choose a reason for hiding this comment

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

LGTM!

@SukkaW SukkaW merged commit 47b97bf into hexojs:master Jul 7, 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.

6 participants