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 hexo plugin in the theme's package.json #4771

Merged
merged 5 commits into from Sep 23, 2021
Merged

Conversation

stevenjoezhang
Copy link
Member

@stevenjoezhang stevenjoezhang commented Sep 7, 2021

What does it do?

Resolve hexojs/hexo-renderer-marked#205 (comment)
See also #4112 #2558 #3890 (comment)

How to test

git clone -b theme-pluginhttps://github.com/hexojs/hexo.git
cd hexo
npm install
npm test

Screenshots

Pull request tasks

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

@coveralls
Copy link

coveralls commented Sep 7, 2021

Coverage Status

Coverage remained the same at 98.282% when pulling ce02bcf on theme-plugin into 042f862 on master.

@stevenjoezhang
Copy link
Member Author

Both the dependencies and devDependencies in the Hexo package.json will be loaded as hexo plugins
Only the dependencies in the theme package.json will be loaded (if installed)

If you have any other suggestions, please reply here.

@jiangtj
Copy link
Member

jiangtj commented Sep 10, 2021

devDependencies are used in the current project for test or build, I think they should be ignored.

I am more concerned about peerdependencies. In npm, it should be used to declare the host environment dependency, which is suitable for our plug-ins. However, the actual performance of peerdependencies is different in npm6 (node ​​14) and npm7. Npm6 will not automatically install peerdependencies dependencies.

https://docs.npmjs.com/cli/v7/configuring-npm/package-json#peerdependencies

I think we should consider support for peerdependencies in future versions

@stevenjoezhang
Copy link
Member Author

I do agree to ignore devDependencies, this will revert #2558

@stevenjoezhang stevenjoezhang requested a review from a team September 19, 2021 06:41
@stevenjoezhang stevenjoezhang changed the title Load hexo plugin in the theme's package.json feat: load hexo plugin in the theme's package.json Sep 19, 2021
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.

Any unit test can be added?

@SukkaW
Copy link
Member

SukkaW commented Sep 19, 2021

Also, maybe we need a separated Node.js 12 PR before this can be merged.

@stevenjoezhang
Copy link
Member Author

Also, maybe we need a separated Node.js 12 PR before this can be merged.

PR opened: #4779

@SukkaW
Copy link
Member

SukkaW commented Sep 20, 2021

@stevenjoezhang #4779 is merged. Would you mind rebasing the PR with the latest master branch? Otherwise, GitHub Action's Lint task will never pass.

Also, Would you mind adding a test case for the PR here? Just to make sure package.json under theme_dir is indeed being read and executed.

SukkaW
SukkaW previously approved these changes Sep 21, 2021
@@ -67,6 +64,10 @@ describe('Load plugins', () => {

after(() => fs.rmdir(hexo.base_dir));

afterEach(() => {
createPackageFile('hexo-another-plugin');
Copy link
Member Author

Choose a reason for hiding this comment

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

This line will reset Hexo package.json, to make sure in the following unit tests, hexo-plugin-test plugin is loaded from theme's package.json, not Hexo package.json

@stevenjoezhang stevenjoezhang requested a review from a team September 21, 2021 09:10
ctx.log.error({err}, 'Plugin load failed: %s', magenta(name));
return Promise.map([ctx.base_dir, ctx.theme_dir], basedir => loadModuleList(ctx, basedir))
.then(([hexoModuleList, themeModuleList]) => {
return Object.entries(Object.assign(hexoModuleList, themeModuleList));
Copy link
Member

Choose a reason for hiding this comment

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

It seems that the plugin defined in the theme will override the one under hexo. The priority in the theme is higher, but the plugin under hexo is added by the user. I think it should have a higher priority.

Copy link
Member Author

Choose a reason for hiding this comment

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

There are two different considerations:

  • The priority in the Hexo package.json is higher: If the theme is not maintained, the users will be able to override the outdated plugins defined in theme package.json
  • The priority in the theme is higher: The plugin in Hexo package.json may not be compatible with the theme, and the theme could provide a proper version for the users

Copy link
Member

Choose a reason for hiding this comment

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

There are two different considerations:

Yesh, but I prefer the first one. After all, if you use the plugin defined by the theme, you only need to remove the relevant plugins in your package.json. In the second option, I will not be able to cover the plugins inside the theme.

Or take a poll? 😂

@SukkaW SukkaW mentioned this pull request Sep 22, 2021
Copy link
Member

@jiangtj jiangtj left a comment

Choose a reason for hiding this comment

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

👍

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