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: add prop to helper extension #4070

Closed
wants to merge 9 commits into from

Conversation

SukkaW
Copy link
Member

@SukkaW SukkaW commented Jan 10, 2020

What does it do?

Fix #4056. Close #3919.

Automatically disable meta_generator filter if meta_generator() is used.

cc @curbengh @jiangtj

How to test

git clone -b extend-helper-prop 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 a review from a team January 10, 2020 08:49
@SukkaW SukkaW changed the title Extend helper prop feat: add prop to helper extension Jan 10, 2020
@coveralls
Copy link

coveralls commented Jan 10, 2020

Coverage Status

Coverage increased (+0.004%) to 97.71% when pulling 1df1d89 on SukkaW:extend-helper-prop into 5c098cc on hexojs:master.

|| data.match(/<meta([\s]+|[\s]+[^<>]+[\s]+)name=['|"]?generator['|"]?/i)) return;
const { config, version } = this;

const needInject = cache.apply('need-inject', () => {
Copy link
Member Author

@SukkaW SukkaW Jan 10, 2020

Choose a reason for hiding this comment

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

I just assume meta[name="generator"] will be inserted to every page if meta_generator() is used or <meta name="generator" content="Hexo [version]"> is manually added in the theme. So it is fine to cache this.

@SukkaW SukkaW requested a review from curbengh January 13, 2020 04:48
@@ -1,13 +1,24 @@
'use strict';

const { Cache } = require('hexo-util');
const cache = new Cache();
Copy link
Member

Choose a reason for hiding this comment

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

Using local val is enough.

let needInject;

...
function hexoMetaGeneratorInject(data) {
  const { config } = this;
  if (typeof needInject !== 'boolean') {
      needInject = !(!config.meta_generator
          || this.extend.helper.getProp('meta_generator')
          || data.match(/<meta([\s]+|[\s]+[^<>]+[\s]+)name=['|"]?generator['|"]?/i));
  }

Copy link
Member Author

Choose a reason for hiding this comment

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

IMHO, use cache.apply here made code more readable since cache.apply support caching return value of given function.

module.exports = metaGeneratorHelper;
module.exports = ctx => {
return () => {
if (!ctx.extend.helper.getProp('meta_generator')) ctx.extend.helper.setProp('meta_generator', true);
Copy link
Member

Choose a reason for hiding this comment

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

Suggest use_meta_generator_helper as the property name.
If meta_generator is used, we don't know what this means.

Copy link
Member Author

Choose a reason for hiding this comment

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

Change to meta_generator_helper_used

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants