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: plugin loading conflict with @vercel/nft #4863

Merged
merged 1 commit into from
Jan 9, 2022

Conversation

SukkaW
Copy link
Member

@SukkaW SukkaW commented Jan 9, 2022

What does it do?

Hexo uses browserify's resolve to resolve a path of a plugin by default, which is incompatible with @vercel/nft. See also: vercel/nft#257

So we use Node.js's built-in require.resolve by default (which is compatible with @vercel/nft), fallback to resolve.sync only when the module is not found (or other errors).

Since there are no other significant behavior differences (from hexo's usage) between resolve.sync and require.resolve, we should drop resolve from dependency in the future.

How to test

git clone -b fix-plugin-resolve 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.

@coveralls
Copy link

coveralls commented Jan 9, 2022

Coverage Status

Coverage decreased (-0.05%) to 98.096% when pulling 83ec33f on SukkaW:fix-plugin-resolve into 9d064d2 on hexojs:master.

@SukkaW SukkaW requested a review from tomap January 9, 2022 07:20
@SukkaW SukkaW mentioned this pull request Jan 9, 2022
53 tasks
Copy link
Contributor

@tomap tomap left a comment

Choose a reason for hiding this comment

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

I approve, but unable to fully understand this change and it's potential impacts

@SukkaW
Copy link
Member Author

SukkaW commented Jan 9, 2022

I approve, but unable to fully understand this change and its potential impacts

The API require.resolve is introduced in Node.js since 0.3.0, however it doesn't support limit scope resolving (only search dependency in the given directories' node_modules, in our case is user's hexo folder) back then. That's probably why package resolve (made by browserify) is used (it provides a base_dir option).

And since Node.js 8.9.0, require.resolve accepted the second argument, which enables to only search under specific given paths.

It shouldn't have any impact at all. And in case there is any, I still use the old resolve package as a fallback, trying to avoid a breaking change.

@SukkaW SukkaW merged commit 4c4bd22 into hexojs:master Jan 9, 2022
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.

4 participants