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(load_config): support theme_dir in node_modules #4112

Merged
merged 2 commits into from
May 6, 2020

Conversation

SukkaW
Copy link
Member

@SukkaW SukkaW commented Jan 31, 2020

What does it do?

The part of #3890

Hexo will read themes/[name] directory first, then search hexo-theme-[name] directory under node_modules. It means the theme under themes directory should have higher priority. In this way no extra theme_from_npm: true configuration is needed.

How to test

git clone -b theme_dir_load 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 tomap, yoshinorin and a team January 31, 2020 12:17
@coveralls
Copy link

Coverage Status

Coverage increased (+0.004%) to 97.704% when pulling 2181124 on SukkaW:theme_dir_load into 667d9a0 on hexojs:master.

@coveralls
Copy link

coveralls commented Jan 31, 2020

Coverage Status

Coverage increased (+0.003%) to 97.745% when pulling 519a312 on SukkaW:theme_dir_load into 714e752 on hexojs:master.

@curbengh
Copy link
Contributor

I don't see any bluebird functions. can the whole load_config be refactored to async, so that existsSync can be avoided?

@SukkaW
Copy link
Member Author

SukkaW commented Feb 25, 2020

@curbengh Refactored into async / await syntax

ctx.theme_dir = themeDirFromNodeModules;
}
ctx.theme_script_dir = join(ctx.theme_dir, 'scripts') + sep;
ctx.theme = new Theme(ctx);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ctx.theme = new Theme(ctx);
ctx.theme = new Theme(ctx);
ctx.theme.ignore = ['**/node_modules/hexo-theme-*/node_modules/**', '**/node_modules/hexo-theme-*/.git/**']

see: https://github.com/hexojs/hexo/blob/master/lib/box/index.js#L11

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Another problem is that whether it is ignored or not, it will affect the performance of watch.

My theme tried to do the same thing in index.js https://github.com/jiangtj/hexo-theme-cake/blob/master/index.js, but when I debug using yarn command (yarn link), Because there are more develop dependencies, hexo cl && hexo s will take longer

May be better solution is to listen only to the specified directory, like source scripts layout languages

Copy link
Member

@stevenjoezhang stevenjoezhang Apr 26, 2020

Choose a reason for hiding this comment

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

This is more symmetrical

  let ignoreCfg;
  if (await exists(themeDirFromThemes)) {
    ctx.theme_dir = themeDirFromThemes;
    ignoreCfg = ['**/themes/*/node_modules/**', '**/themes/*/.git/**'];
  } else if (await exists(themeDirFromNodeModules)) {
    ctx.theme_dir = themeDirFromNodeModules;
    ignoreCfg = ['**/node_modules/hexo-theme-*/node_modules/**', '**/node_modules/hexo-theme-*/.git/**'];
  }
  ctx.theme_script_dir = join(ctx.theme_dir, 'scripts') + sep;
  ctx.theme = new Theme(ctx);
  ctx.theme.ignore = ignoreCfg;

And https://github.com/hexojs/hexo/blob/master/lib/box/index.js#L11 can be removed

Actually .git is not uploaded to npm by default

Copy link
Member Author

Choose a reason for hiding this comment

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

@jiangtj @stevenjoezhang This could be done in another PR then.

Copy link
Member

Choose a reason for hiding this comment

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

I agree that another PR can be used to deal with the efficiency issues caused by watch(). After resolving the conflict, this PR can be merged

Copy link
Member Author

Choose a reason for hiding this comment

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

@stevenjoezhang Rebased.

tomap
tomap previously approved these changes May 2, 2020
@SukkaW
Copy link
Member Author

SukkaW commented May 5, 2020

Rebased to latest master.

@SukkaW SukkaW requested a review from jiangtj May 5, 2020 07:07
@SukkaW SukkaW merged commit 515cf6f into hexojs:master May 6, 2020
@SukkaW SukkaW mentioned this pull request Jul 25, 2020
22 tasks
@sergeyzwezdin
Copy link

@SukkaW Does it support scoped package name? I.e. @somescope/hexo-theme-mytheme.

@SukkaW
Copy link
Member Author

SukkaW commented Jul 29, 2020

Does it support scoped package name

Currently, no.

@sergeyzwezdin
Copy link

Any plans to introduce it?

@SukkaW
Copy link
Member Author

SukkaW commented Jul 29, 2020

Any plans to introduce it?

You can opened a new issue for Feature Request so that we can track it.

@sergeyzwezdin
Copy link

You can opened a new issue for Feature Request so that we can track it.

Added #4496

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.

7 participants