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: buildCommand behavior in a monorepo #60

Merged
merged 3 commits into from
Jul 15, 2020
Merged

Conversation

marcelltoth
Copy link
Contributor

Assuming a fixed node_modules path breaks within a monorepo powered by yarn workspaces.

image

(because the dep is hoisted up to .../source/stoplight/elements/node_modules/.bin/)

There is no point in doing the resolution ourselves anyway. We can allow the shell to do the resolution based on PATH, which will then work regardless of where the dependency is located exactly.

@marcelltoth marcelltoth self-assigned this Jul 13, 2020
@marcelltoth marcelltoth requested review from marbemac and a team July 13, 2020 14:30
Copy link
Contributor

@billiegoose billiegoose left a comment

Choose a reason for hiding this comment

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

Too risky. That would allow commands installed globally to work too, which would result in people forgetting to add a dependency because it would work on their machine. (Plus, it's always a terrible idea to rely on $PATH in my opinion.)

Let's do some kind of upword directory search starting at cwd for node_modules/.bin/${baseCommand}

@marcelltoth
Copy link
Contributor Author

Too risky. That would allow commands installed globally to work too, which would result in people forgetting to add a dependency because it would work on their machine. (Plus, it's always a terrible idea to rely on $PATH in my opinion.)

Let's do some kind of upword directory search starting at cwd for node_modules/.bin/${baseCommand}

What about yarn run ${x}?

Or this library: https://github.com/saltyshiomix/resolve-as-bin

requre.resolve acts weird for some reason.

image

@marcelltoth marcelltoth requested a review from billiegoose July 14, 2020 15:57
@billiegoose
Copy link
Contributor

What about yarn run ${x}?

That only works on scripts in package.json, not actual binaries AFAICT.

Or this library: https://github.com/saltyshiomix/resolve-as-bin

That library uses spawn('npm bin') to find the .bin directory, which won't work for monorepos either.

I bet I can find something...

@billiegoose
Copy link
Contributor

https://github.com/sindresorhus/find-up looks good. I like sindresorhus's stuff.

@billiegoose
Copy link
Contributor

@marcelltoth See if my commit works for your monorepo use case.

@marcelltoth
Copy link
Contributor Author

@marcelltoth See if my commit works for your monorepo use case.

Awesome! Works perfectly.

@marcelltoth marcelltoth merged commit 4fe19dd into master Jul 15, 2020
@marcelltoth marcelltoth deleted the fix/monorepo-build branch July 15, 2020 11:37
@stoplight-bot
Copy link
Collaborator

🎉 This PR is included in version 8.2.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

3 participants