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

Support Yarn Workspaces #27

Merged
merged 4 commits into from
Feb 27, 2018
Merged

Support Yarn Workspaces #27

merged 4 commits into from
Feb 27, 2018

Conversation

wyattjoh
Copy link
Contributor

If docs are included as a part of the primary application via yarn workspaces (See https://yarnpkg.com/lang/en/docs/workspaces/) the module resolution for the main hexo package fails, because it is nested above.

This PR addresses this issue by replacing the current custom module resolution with the resolve which implements the resolve algorithm.

This, in my opinion, fits the design requirements as well as extending it to allow for the yarn workspace issue without much hassle.

@coveralls
Copy link

coveralls commented Feb 21, 2018

Coverage Status

Coverage remained the same at 85.816% when pulling 0121a47 on wyattjoh:master into f2f7364 on hexojs:master.

@wyattjoh
Copy link
Contributor Author

This one is blocking a release for us @NoahDragon, any chance it could get merged soon?

@@ -86,7 +86,7 @@ entry.version = require('../package.json').version;

function loadModule(path, args) {
return Promise.try(function() {
var modulePath = pathFn.join(path, 'node_modules', 'hexo');
var modulePath = resolve.sync('hexo', { basedir: path });
Copy link
Contributor

Choose a reason for hiding this comment

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

It is sufficient to use builtin require.resolve since resolve implements require.resolve for browser-side support and hexo does not, at least officially, support browser environment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough! At the time I discovered the resolve package, node didn't export the right stuff. I've patched it!

@kgardnr
Copy link

kgardnr commented Feb 27, 2018

Hey @JLHwung could you check out our changes and let us know if we can get this merged soon? Thanks so much!

@JLHwung
Copy link
Contributor

JLHwung commented Feb 27, 2018

Hi @wyattjoh , as the paths option is supported on node.js 8.9.0+, could you revert 618286b? I should have investigated the paths option support.

@wyattjoh
Copy link
Contributor Author

That issue should be resolved @JLHwung!

@JLHwung JLHwung merged commit bbd263e into hexojs:master Feb 27, 2018
@kgardnr
Copy link

kgardnr commented Feb 27, 2018

Thanks @JLHwung! Would there be any chance you can look at this similar one? hexojs/hexo#3045

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