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

The proposal of installing theme through npm #3890

Open
SukkaW opened this issue Nov 29, 2019 · 11 comments
Open

The proposal of installing theme through npm #3890

SukkaW opened this issue Nov 29, 2019 · 11 comments

Comments

@SukkaW
Copy link
Member

@SukkaW SukkaW commented Nov 29, 2019

The original discussion is at #2471

Here is my proposal:

  1. We should not treat hexo-theme- prefixed package as a hexo plugin. See @tomap 's commit: master...tomap:feature/themeAsPackagediff-3d0f701643104f3b0e388b8f6a9a7f3eR38
  2. Users are still require to specify their theme's name in _config.yml, because...
  3. 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.
  4. theme_config in _config.yml looks good, but it is better to have a separate theme config. We could introduce _config.[name].yml. Currently, theme_config has higher priority than _config.yml under theme directories. After bring up _config.[name].yml, I suggest the theme_config in _config.yml > _config.[name].yml > _config.yml under theme directories which will retain highest priority for theme_config.
  5. Also, we need to fix the issue that some plugins can not read theme config from theme_config: #2471 (comment)

IMHO, we could easily bring up this feature without any Breaking Changes: the theme structure, current configurations and setup will not be affected. Yes, I mean we can even bring up this feature during Hexo 4.x development.

cc @hexojs/core @jiangtj

@SukkaW

This comment has been minimized.

Copy link
Member Author

@SukkaW SukkaW commented Nov 29, 2019

FYI:
Currently there are more than 50 packages on npm has hexo-theme- prefixed: https://www.npmjs.com/search?q=hexo-theme (Others are eslint-config-theme-next or -theme-hexo suffixed)

@stevenjoezhang

This comment has been minimized.

Copy link
Contributor

@stevenjoezhang stevenjoezhang commented Nov 29, 2019

We should not treat hexo-theme- prefixed package as a hexo plugin.

Is there a good way to handle this situation?
Plugin: https://www.npmjs.com/package/hexo-theme-next-anchor
Theme: https://www.npmjs.com/package/hexo-theme-next

@SukkaW SukkaW pinned this issue Nov 29, 2019
@tomap

This comment has been minimized.

Copy link
Contributor

@tomap tomap commented Nov 29, 2019

We should not treat hexo-theme- prefixed package as a hexo plugin.

Is there a good way to handle this situation?
Plugin: https://www.npmjs.com/package/hexo-theme-next-anchor
Theme: https://www.npmjs.com/package/hexo-theme-next

We could contact all owners of hexo-theme-* on npm.org when they are not themes

The package hexo-theme-next-anchor is indeed a plugin.
We should suggest the owner to change it's name (21 downloads per week)

For the package hexo-theme-next, it is a theme. so, in this case, no problem

@tomap

This comment has been minimized.

Copy link
Contributor

@tomap tomap commented Nov 29, 2019

@SukkaW I edited your issue to add numbers for each items

for 1. indeed, that is the pre requisite
but in the list of packages on npm, as you mentioned, there are multiple packages prefixed "hexo-theme-*"
I looked at each of them, and I believe that none are plugins, except one:
https://www.npmjs.com/package/hexo-theme-next-anchor
=> not a big deal. we'll contact the owner
And we have to decide is that means waiting for Hexo 5 or a smaller 4.1 could be ok

for 2. ok

for 3. ok, but it will make the code a bit more complex because the theme_dir will be more complex to determine (not a big deal either)

for 4. ok also, but this is a bonus

for 5. this is not really a bonus, because without that, we cannot customize .less (and I believe it is probably the same for .sass

If I have time, I'll start with 3.
(I don't have a lot of time those days, but maybe...)

@jiangtj

This comment has been minimized.

Copy link

@jiangtj jiangtj commented Dec 2, 2019

hexo-theme-* is treated as a theme by default, so this is not a problem, just contact the owner to modify it.
@stevenjoezhang rename hexo-theme-next-anchor to hexo-next-anchor, maybe better. Or load
all hexo-theme-next-* plugin manual.
https://github.com/hexojs/hexo/blob/master/lib/hexo/load_plugins.js#L51

@stevenjoezhang

This comment has been minimized.

Copy link
Contributor

@stevenjoezhang stevenjoezhang commented Dec 2, 2019

IMHO, distinguishing between a theme and a plugin is not complicated, you just need to determine whether a layout directory exists - it is a pity to deprecate some modules for this reason.

cc @1v9

@paulkre

This comment has been minimized.

Copy link

@paulkre paulkre commented Dec 9, 2019

Great proposal! Would love to use this feature.

@SukkaW

This comment has been minimized.

Copy link
Member Author

@SukkaW SukkaW commented Dec 9, 2019

you just need to determine whether a layout directory exists - it is a pity to deprecate some modules for this reason.

Another idea is only to exclude hexo-theme-[config.theme] from being loaded as a plugin. But it means installing multiple themes will become impossible.

cc @stevenjoezhang @tomap @jiangtj

@tomap

This comment has been minimized.

Copy link
Contributor

@tomap tomap commented Dec 9, 2019

Also, we need to fix the issue that some plugins can not read theme config from theme_config: #2471 (comment)

I just retested the item 5. and it seems to work without my changes. I must have tested wrong :)

@jiangtj

This comment has been minimized.

Copy link

@jiangtj jiangtj commented Dec 10, 2019

Another idea is only to exclude hexo-theme-[config.theme] from being loaded as a plugin. But it means installing multiple themes will become impossible.

This brings me to another idea. We can configure in the configuration file, which need to be excluded

@SukkaW

This comment has been minimized.

Copy link
Member Author

@SukkaW SukkaW commented Dec 10, 2019

@tomap Yeah, theme_config is supposed to be included in Locals.prototype.theme.

Locals.prototype.theme = Object.assign({}, config, theme.config, config.theme_config);

@jiangtj It would be more complex for a user to configure it.

This brings me to another idea. We can configure in the configuration file, which need to be excluded

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.