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

Replace default swig engine with nunjucks #2903

Merged
merged 4 commits into from
Apr 26, 2020

Conversation

thom4parisot
Copy link
Member

@thom4parisot thom4parisot commented Dec 11, 2017

It feels reasonable to remove swig due to its deprecation and various issues reporting it: #2895, #1593, #1625, #1915, #2295

This PR aims to:

What do you think?

@coveralls
Copy link

coveralls commented Dec 11, 2017

Coverage Status

Coverage decreased (-0.2%) to 96.983% when pulling dbabd3b on oncletom:feature/remove-swig into c8d47b8 on hexojs:master.

@kovtunos
Copy link

Replace swig with nunjucks v3.

@thom4parisot
Copy link
Member Author

Thanks @kovtunos.

Shall we still process .swig files with nunjucks? By default I would not do it but project wise, I don't know if it is relevant.

@coveralls
Copy link

coveralls commented Dec 13, 2017

Coverage Status

Coverage decreased (-0.4%) to 96.794% when pulling d994d52 on oncletom:feature/remove-swig into c8d47b8 on hexojs:master.

@thom4parisot
Copy link
Member Author

thom4parisot commented Dec 13, 2017

At the moment all swig tests seem to pass with nunjucks as a renderer. Except one which is related to the for override (override "for" tag in test/scripts/renderers/swig.js).

The test is ran against this data structure:

var data = {
      arr: {
        toArray() {
          return [1, 2, 3];
        }
      }
    };

Is that something we want to keep? Is that related to a Collection object (like posts) which has a toArray() method to move out of the Warehouse realm?

I would find it less hacky to write {% for post in posts | list %} for example. I would understand we would like to keep a maximum compatibility

@coveralls
Copy link

coveralls commented Dec 13, 2017

Coverage Status

Coverage decreased (-0.06%) to 97.17% when pulling 1e85e39 on oncletom:feature/remove-swig into c8d47b8 on hexojs:master.

@NoahDragon NoahDragon added this to the 4.0 milestone Dec 16, 2017
@NoahDragon NoahDragon added the enhancement New feature or request label Dec 16, 2017
@NoahDragon
Copy link
Member

Hi @oncletom , my two cents, we should keep the backward capability. The most popular Hexo theme NexT (https://github.com/iissnan/hexo-theme-next) is still using the swig. However, if the change could work on NexT, then we could merge this change for Hexo 4.

@NoahDragon NoahDragon mentioned this pull request Dec 16, 2017
53 tasks
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 97.201% when pulling 1478019 on oncletom:feature/remove-swig into c8d47b8 on hexojs:master.

1 similar comment
@coveralls
Copy link

coveralls commented Dec 17, 2017

Coverage Status

Coverage decreased (-0.03%) to 97.201% when pulling 1478019 on oncletom:feature/remove-swig into c8d47b8 on hexojs:master.

@coveralls
Copy link

coveralls commented Dec 17, 2017

Coverage Status

Coverage increased (+0.005%) to 97.231% when pulling 7179a08 on oncletom:feature/remove-swig into c8d47b8 on hexojs:master.

@coveralls
Copy link

coveralls commented Dec 17, 2017

Coverage Status

Coverage decreased (-0.02%) to 97.742% when pulling 2cdecbf on oncletom:feature/remove-swig into fd68a1b on hexojs:master.

@huiwang
Copy link

huiwang commented Jan 5, 2018

hi, this is an exciting proposal. it will make it possible to use async helper in themes.

@charsi
Copy link

charsi commented Jan 13, 2018

The next theme is now hosted here -- https://github.com/theme-next/hexo-theme-next/

@NoahDragon
Copy link
Member

@oncletom, is this still working in progress?

@thom4parisot
Copy link
Member Author

Hi @NoahDragon!

I would say most of the work is done. At the moment nunjucks is configured to handle .swig files but that can be changed/removed/put under a deprecation notice setting.

Next step would be to try it out with hexo-theme-next to make sure it works, either out of the box or with additional fixes.

I don't know if anybody has time to do it, or to test and to report the issues in this thread.

Otherwise let me know if there are any useful pointers to integrate this contribution to the 4.x branch sooner or later 🙂

@NoahDragon
Copy link
Member

@oncletom Thanks for the update. I will test this change when I have a chance. 😄

@djmtype
Copy link

djmtype commented Jun 27, 2018

What's the status on using Nunjucks? I'm only pointing this out because the official site no longer mentions it along side of pug, ejs, haml and swig. https://hexo.io/docs/themes.html#layout Besides, it seems *.ejs is currently the adopted template language of Hexo.

@chenbojian
Copy link

it looks like nunjunks is merged to node itself. And it cause some errors to install hexo on node 11.3.0. #3380

@tomap
Copy link
Contributor

tomap commented Dec 5, 2018

@chenbojian it's unrelated to this issue

@thom4parisot
Copy link
Member Author

Well, I don't know—I can put more efforts into it.

I can also make it a separate plugin if needed and if none already exist to do the job.

Any feedback from the @hexojs team?

@yoshinorin
Copy link
Member

I can also make it a separate plugin

IMHO, should merge this change and bump up to hexo version to V4. No need separate to plugin.

@chawyehsu
Copy link
Contributor

Any progress? @oncletom maybe you need to resolve the conflicts.

@chawyehsu
Copy link
Contributor

chawyehsu commented Mar 27, 2019

I found that it's impossible to render swig template with nunjucks engine, since they have minor different syntax, see https://medium.com/engineers-optimizely/js-templating-transitioning-from-swig-to-nunjucks-ac0e94d1794b

Shall we still process .swig files with nunjucks? By default I would not do it but project wise, I don't know if it is relevant.

So either totally remove swig, or don't add nunjucks renderer. You shouldn't render swig with nunjucks.

@thom4parisot
Copy link
Member Author

thom4parisot commented Mar 27, 2019

Interesting, thanks for sharing this conclusion.

Shall we not render both of them and make it a separate renderer instead?

@thom4parisot
Copy link
Member Author

thom4parisot commented Jan 17, 2020

Sorry, I made a typo: I was not referring to .js files but .j2 files 😅

Switching to Nunjucks was already a breaking change. I have no idea if hexo-renderer-swig is on par with the previously embedded swig rendering. I'm not convinced it is part of the scope of this PR, but I'm happy to open an additional PR to the default hexo-renderer-swig to maintain a feature parity.

SukkaW
SukkaW previously approved these changes Jan 23, 2020
Copy link
Member

@SukkaW SukkaW left a comment

Choose a reason for hiding this comment

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

LGTM!

Pending hexo-renderer-nunjucks joins hexo official plugins and releases a new version.

@NoahDragon Please add @oncletom to plugin team at GitHub org.

package.json Outdated Show resolved Hide resolved
@thom4parisot
Copy link
Member Author

@SukkaW I invited you as a collaborator on the repo so as you can do the transfer — maybe, if that works?

A new version of hexo-renderer-nunjucks should be published — a v2.0.0 to avoid any conflict with previous installs of the same-npm-but-different-repo renderer).

@SukkaW
Copy link
Member

SukkaW commented Jan 24, 2020

I invited you as a collaborator on the repo so as you can do the transfer — maybe, if that works?

Emmm, nope. I have accepted the invitation, but collaborator can not access repo's settings page.

BTW, for NPM you could add curben as an package owner. Currently @curbengh is the publisher for all hexo related packages.

stevenjoezhang
stevenjoezhang previously approved these changes Feb 26, 2020
@SukkaW
Copy link
Member

SukkaW commented Feb 26, 2020

@oncletom I have asked Hexo orgs owner to send your invitation to join hexojs. Maybe then you will have enough permission to transfer the repo then.

@thom4parisot
Copy link
Member Author

thom4parisot commented Feb 26, 2020

Okay, thank you @SukkaW.

Also, now I think about it, if you have the correct permissions, you can create an empty repo in the hexojs org, and push the repo content to it. The end result is the same (working code and commits history).

Regarding the npm repo itself (https://www.npmjs.com/package/hexo-renderer-nunjucks), I don't have write access to it. These persons can add curben as a package owner:

[ "tommy351", "ertrzyiks", "jlhwung", "abnerchou", "hexobot" ]

@yoshinorin
Copy link
Member

yoshinorin commented Mar 3, 2020

@oncletom
I sent you an invitation to join the hexo org.

Thanks :)

@SukkaW
Copy link
Member

SukkaW commented Mar 3, 2020

Currently hexo's built-in swig renderer is using some ES6 syntax that hexo-renderer-swig doesn't have yet. I have opened a new PR for that: hexojs/hexo-renderer-swig#23


These persons can add curben as a package owner:

@NoahDragon Would you mind adding @curbengh (curben) as the owner of hexo-renderer-nunjucks at npm?

@thom4parisot
Copy link
Member Author

Thank you 🙂 I have transferred the repo. It now lives as hexojs/hexo-renderer-nunjucks 🙌

I added a note in the README to automatically publish a release on npm, for every new Git tag pushed to GitHub.

Once the env variables are set, creating a v2.0.0 release will publish the updated package.

Then this comment can be addressed, by committing the suggestion.

I have updated this PR description to reflect these steps.

@SukkaW
Copy link
Member

SukkaW commented Mar 30, 2020

Once the env variables are set, creating a v2.0.0 release will publish the updated package.

@JLHwung @NoahDragon As you are the owners of hexo-renderer-nunjucks, could you please add @oncletom as the owner of the hexo-renderer-nunjucks at npm? So that a newer version could be released.

@SukkaW
Copy link
Member

SukkaW commented Apr 25, 2020

@ertrzyiks Hi, it seems that you are an owner of hexo-renderer-nunjucks. Would you mind adding @oncletom (oncletom at npm) to hexo-renderer-nunjucks?

@ertrzyiks
Copy link
Contributor

@SukkaW @oncletom added

@thom4parisot thom4parisot dismissed stale reviews from stevenjoezhang and SukkaW via 2cdecbf April 26, 2020 07:50
@thom4parisot
Copy link
Member Author

Thank you @ertrzyiks, as well as @SukkaW for chasing this down 🙂

hexo-renderer-nunjucks@2 is live on npm, and this PR has been updated accordingly 👍

@SukkaW SukkaW merged commit 78b086f into hexojs:master Apr 26, 2020
@thom4parisot thom4parisot deleted the feature/remove-swig branch April 26, 2020 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.