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(hexo): merge theme_config before generation #4360

Merged
merged 4 commits into from
Jul 9, 2020

Conversation

SukkaW
Copy link
Member

@SukkaW SukkaW commented Jun 16, 2020

What does it do?

#4120, #4120 (comment), #3890 (comment)

This PR closes #3890.

How to test

git clone -b theme_config_ctx 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 segayuu, stevenjoezhang, curbengh and a team June 16, 2020 09:04
@SukkaW SukkaW added this to the 5.0.0 milestone Jun 16, 2020
@tomap
Copy link
Contributor

tomap commented Jun 16, 2020

Hi,

I'm not sure why you are moving the code? Is it done sooner with your code?
Or later?

Why?

@SukkaW
Copy link
Member Author

SukkaW commented Jun 16, 2020

@tomap Technically, sooner. this._genterateLocals build Locals in this._generate, while this._generate is called after this.load. While in this PR I move the deepMerge into this.load.

More details please see #4120 (comment) . In short, @stevenjoezhang want the plugin being able to read config.theme_config from theme.config.

@stevenjoezhang
Copy link
Member

Yes, this is very important for theme developers. Otherwise additional code is needed to ensure that the configuration in theme_config is loaded correctly.
See also
theme-next/hexo-theme-next@1b0b53a
https://github.com/theme-next/hexo-theme-next/blob/a7a948a08f48dbe48879e277703d75706aec18e5/scripts/events/lib/config.js#L34

@stevenjoezhang
Copy link
Member

hexo/lib/hexo/index.js

Lines 304 to 340 in d117819

load(callback) {
return loadDatabase(this).then(() => {
this.log.info('Start processing');
return Promise.all([
this.source.process(),
this.theme.process()
]);
}).then(() => this._generate({cache: false})).asCallback(callback);
}
watch(callback) {
let useCache = false;
const { cache } = Object.assign({
cache: false
}, this.config.server);
if (this.env.cmd.startsWith('s') && cache) {
// enable cache when run hexo server
useCache = true;
}
this._watchBox = debounce(() => this._generate({cache: useCache}), 100);
return loadDatabase(this).then(() => {
this.log.info('Start processing');
return Promise.all([
this.source.watch(),
this.theme.watch()
]);
}).then(() => {
this.source.on('processAfter', this._watchBox);
this.theme.on('processAfter', this._watchBox);
return this._generate({cache: useCache});
}).asCallback(callback);
}

I confirm that this patch works fine with hexo g. However, in hexo s the _generate method is usually called by watch (line 325, 338), instead of load (line 312)
Is it possible to put the relevant code directly into the _generate method?

@SukkaW
Copy link
Member Author

SukkaW commented Jun 16, 2020

Is it possible to put the relevant code directly into the _generate method?

@stevenjoezhang Then any filter related with post rendering (like before_post_render) will be ignored.


Updated

@stevenjoezhang

Merge theme config during watch() is now supported. Reloading the theme config is supported as well (see the test case added): b23fd52

@coveralls
Copy link

coveralls commented Jun 16, 2020

Coverage Status

Coverage remained the same at 97.805% when pulling 1831bd1 on SukkaW:theme_config_ctx into 3363a8c on hexojs:master.

@curbengh
Copy link
Contributor

Merge theme config during watch() is now supported. Reloading the theme config is supported as well (see the test case added)

My test config:

_config.yml

theme_config:
  foo:
    bar: 'bax'

theme's _config.yml

foo:
  baz: 'bos'

article.ejs (to display theme config)

<%- JSON.stringify(theme.foo, null, 2) %>

In hexo s, after I change the value of theme.foo.baz in theme's _config.yml and reload the page, it crash with undefined property. No effect when changing theme.foo.bar in _config.yml, reloading the page doesn't show the updated value.

@SukkaW
Copy link
Member Author

SukkaW commented Jun 20, 2020

Reloading the page doesn't show the updated value.

@curbengh It is expected behavior. Because site's _config.yml won't reload. Only theme's _config.yml will.

after I change the value of theme.foo.baz in theme's _config.yml and reload the page, it crash with undefined property

Weird. Doesn't happen on my local machine.

@SukkaW
Copy link
Member Author

SukkaW commented Jun 20, 2020

@curbengh Could you please re-verify reloading theme's config?

$ npm i sukkaw/hexo#theme_config_ctx

Then

# _config.yml
theme_config:
  foo:
    bar: 'bax'
# theme's _config.yml
foo:
  baz: 'bos'

Add following line to the theme template:

<% console.log(theme.foo) %>

hexo s should output:

{ baz: 'bos', bar: 'bax' }

Update theme's config:

# theme's _config.yml
foo:
  baz: 'box'

Then reload the page should output:

{ baz: 'box', bar: 'bax' }

@curbengh
Copy link
Contributor

curbengh commented Jun 21, 2020

This time I tried showing everything inside theme object,

<% console.log(JSON.stringify(theme, null, 2)) %>

After I changed the theme.foo.baz in the theme's config and reload the page, the theme object is left with theme.foo.bar only. Only theme_config is loaded, all values in theme's _config are removed.


Looks like my machine's issue. hexo s watch/live reload doesn't work for me, when I change a post's content and reload, that (modified) post's content will be empty, other (untouched) posts are fine. Similar to #4271.

@SukkaW
Copy link
Member Author

SukkaW commented Jun 21, 2020

@curbengh So it appears that it is not the issue if the PR though. We could dig into the watcher later.

Have you run unit test? Is it passed?

@curbengh
Copy link
Contributor

curbengh commented Jun 22, 2020

I can confirm it's my machine's issue, not even downgrading to v3.9 helps. I recently migrate Ubuntu -> Solus and I don't recall having this watch issue in Ubuntu. Anyway, I'll debug on my own, don't want to distract this PR.

Unit test passed on Node 12 & 14 in my machine. I still prefer to have a theme dev @stevenjoezhang to verify this PR.

@SukkaW
Copy link
Member Author

SukkaW commented Jun 30, 2020

@curbengh @stevenjoezhang

I have added a test case, using before_post_render, after_post_render, before_generate filters to check if theme.config has been merged.

@jerryc127
Copy link

jerryc127 commented Aug 9, 2020

主題配置文件已經默認了一些配置
例如

item:
  - qq
  - wechat

我的_config.xxxxx.yml
更改配置

item:
  - twitter

結果 是生成 qq wechat twitter
而不是我修改的 twitter

按道理 如果_config.xxxxx.yml配置已有 應該按照_config.xxxxx.yml的内容為准,因爲它優先性高.
這是本來設計就是這樣麽

@curbengh
Copy link
Contributor

curbengh commented Aug 9, 2020

Priority is not relevant if a key doesn't have duplicate subkeys, they will be merged, not replaced.

# themes/xxxx/_config.yml
item:
  lorem: 'a'
  ipsum: 'b'
# _config.xxxx.yml
item:
  dolor: 'c'
console.log(theme.config)
item: {
  lorem: 'a',
  ipsum: 'b',
  dolor: 'c'
}

A subkey will only be replaced if there is duplicate:

# themes/xxxx/_config.yml
item:
  lorem: 'a'
  ipsum: 'b'
# _config.xxxx.yml
item:
  lorem: 'c'
console.log(theme.config)
item: {
  lorem: 'c',
  ipsum: 'b'
}

@jerryc127
Copy link

你这个举例 我明白
问题是我那例子属于subkey么

item:
  - qq
  - wechat

qq wechat 这个配置都是属于 item这个key的。 qq和wechat都是item 的值,就好比c是lorem的值,b是ipsum的值,
那这样 我的这个例子不是应该替换么。。

难道以 -xxx -xxx -xxx 这种形式赋值 都会被识别为几个subkey?

@curbengh
Copy link
Contributor

In your example, item has array value. array value is merged.

const { deepMerge } = require('hexo-util')

const config = {
  theme_config: {
    item: ['a', 'b']
  }
}

const theme = {
  config: {
    item: ['c']
  }
}

theme.config = deepMerge(theme.config, config.theme_config);

console.log(theme.config)
// { item: [ 'c', 'a', 'b' ] }

If you want to replace the original item, you have to remove it.

@YunYouJun
Copy link

Why not overwrite array instead of combine arrays?

Is there any special reason? I think for themes, overwriting array is more suitable to providing some example configurations without disturbing the user.

Or just provide a configuration to decide whether to overwrite the array or merge the array?

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.

The proposal of installing theme through npm
7 participants