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: disable deep merging theme configs by default #4154

Closed
wants to merge 1 commit into from

Conversation

curbengh
Copy link
Contributor

What does it do?

Continuation of #3967. Requested by #3967 (comment) @Teekivi
deep merging theme.config and config.theme_config is considered less common use case.

cc #3964 @cmpute

Introduce a new option:

deepmerge_theme_configs: false

How to test

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

Screenshots

Pull request tasks

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

@SukkaW
Copy link
Member

SukkaW commented Feb 25, 2020

I don't think it is a common case.

theme_config is designed to merge with existed theme's _config.yml and only override existed value, which deep merge is doing exactly what it should do. And I believe the case @Teekivi provided is
in fact less common.

IMHO, the case @Teekivi provided is due to the bad-designed theme (which provides default items instead of giving example in _config.yml), not the fault of Hexo. After we bring up deep merge behavior, it seems that only hexo-theme-hermes is facing the issue?


Providing a case:

# theme's _config.yml
a:
  b:
    menu:
      d: 'e'
      f: 'g'
    sidebar:
      h: 'i'
# _config.yml
theme_config:
  a:
    b:
      menu:
        d: 'e'

Then they will be no sidebar config left. The @Teekivi cases only exists for theme's _config.yml looks like this:

# theme's _config.yml
# No less than 2 levels, which means override any 1st level won't affect other configurations 
a:
  b: e
  c: f
d: g

@Teekivi
Copy link

Teekivi commented Feb 25, 2020

@SukkaW I'm quite new to the Hexo world, so you are probably right about my problem not being very common.

I understand that the "case" you provided is not meant as a good example, but it does show the limitations posed by default deep merge. As I said in #3967 (comment) deep merge makes it impossible to reorder or omit predefined dictionary/menu items solely via theme_config. So, in your example, you cannot change the order of d and f or omit one or both of them. That is - without changing theme source code.

I think that deep merge brings a lot more good than bad. But in my case, I experienced the less prevalent downside of it. So I think that as a possibility, we could expect future theme development to take the new behavior into account and not change it. Or we could make it globally opt-out or opt-in (the latter being this case), or figure out a good way to enable/disable deep merge selectively.

@curbengh
Copy link
Contributor Author

curbengh commented Mar 6, 2020

IMHO, the case provided is due to the bad-designed theme (which provides default items instead of giving example in _config.yml)

👍

figure out a good way to enable/disable deep merge selectively

I could introduce an option, which is a reverse of this PR. But that option is more of a workaround for a theme which has invalid defaults, so the option wouldn't be useful.

@curbengh curbengh closed this Mar 6, 2020
@curbengh curbengh deleted the optional-merge-theme branch March 6, 2020 00:18
@stevenjoezhang
Copy link
Member

stevenjoezhang commented Apr 28, 2020

I have encountered this problem in the theme NexT: iissnan/hexo-theme-next#1861 theme-next/hexo-theme-next#878
The solution is not to preset any menu items - all default options should be commented. Maybe we need to remind all theme developers here to pay attention to this: https://github.com/hexojs/hexo-theme-unit-test

hexojs/hexo-theme-landscape#102

@FossPrime
Copy link

FossPrime commented Jun 26, 2021

I have encountered this problem in the theme NexT: iissnan/hexo-theme-next#1861 theme-next/hexo-theme-next#878
The solution is not to preset any menu items - all default options should be commented. Maybe we need to remind all theme developers here to pay attention to this: https://github.com/hexojs/hexo-theme-unit-test

hexojs/hexo-theme-landscape#102

This is an issue on the landscape default theme... All the menu items are loaded by default.

I think there are a few compounding problems

  • the schema should allow object values for widgets: [{ enabled: false, name: tag_cloud } ]
  • we should be able to choose which config we wish to inherit from using Yaml. We can inherit the full config for demos, and the minimal one for serious work.

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

Successfully merging this pull request may close these issues.

5 participants