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

fix(theme): deep merge configs #3967

Merged
merged 2 commits into from
Dec 16, 2019
Merged

Conversation

curbengh
Copy link
Contributor

@curbengh curbengh commented Dec 15, 2019

What does it do?

Fixes #3964

How to test

cc @cmpute

# themes/xxx/_config.yml
a:
  c: 2
# _config.yml
theme_config:
  a:
    b: 3
package.json
-  "hexo": "^4.1.1",
+  "hexo": "curbengh/hexo#deepmerge_theme_config"
  1. rm package-lock.json && rm -rf node_modules/
  2. npm install
  3. hexo clean && hexo server (or hexo generate if relevant)
  4. Verify theme.a should have { b: 3, c: 2}

Maintainers:

git clone -b deepmerge_theme_config https://github.com/curbengh/hexo.git
cd hexo
npm install
npm test

Pull request tasks

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

@coveralls
Copy link

coveralls commented Dec 15, 2019

Coverage Status

Coverage decreased (-0.006%) to 97.133% when pulling 9b401fd on curbengh:deepmerge_theme_config into 2c50d3b on hexojs:master.

@cmpute
Copy link

cmpute commented Dec 15, 2019

Thanks!

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!

@curbengh
Copy link
Contributor Author

Verified in a test blog.

@curbengh curbengh merged commit 457fcfc into hexojs:master Dec 16, 2019
@curbengh curbengh deleted the deepmerge_theme_config branch December 16, 2019 01:26
stevenjoezhang referenced this pull request in theme-next/hexo-theme-next Dec 18, 2019
@SukkaW SukkaW mentioned this pull request Dec 21, 2019
1 task
@Teekivi
Copy link

Teekivi commented Feb 6, 2020

Is it possible to (selectively) opt out of deep merging theme configs? I am using a theme called Hermes which uses a dictionary for its menu. It has some menu items predefined. I want to replace all of those with my items. Due to deep merging, I end up with a menu that contains both the predefined and my items.

@curbengh
Copy link
Contributor Author

curbengh commented Feb 22, 2020

would modifying the theme's config.yml (instead of theme_config) resolve the issue? or remove the default value.

@Teekivi
Copy link

Teekivi commented Feb 24, 2020

Yes, modifying the theme's config.yml would indeed resolve issue. But having to modify the theme's source code kind of defeats the purpose of theme_config. As the documentation puts it:

Instead of forking a theme, and maintaining a custom branch with your settings, you can configure it from your site’s primary configuration file.

Enforcing deep merge of configs without making it opt-in or opt-out makes using theme_config cumbersome or in some cases impossible with (I believe) quite a lot of pre-existing themes.

I imagine that lots of themes come with example menus etc to showcase them out-of-the-box. If a menu is defined using a dictionary and the user happens to want to use exactly the same menu item names and in that same order as the predefined one, the user is in luck. In other cases, they'll want to omit and/or reorder predefined items, both of which are currently impossible via theme_config due to deep merge.

@yezige
Copy link

yezige commented Apr 28, 2020

awesome

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.

Theme config is not deeply updated
6 participants