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.

Sign up for GitHub

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

Jump to bottom

fix(commands): add -W to yarn add when in yarn workspaces root #121

Merged
merged 3 commits into from
Jan 6, 2021
Merged

fix(commands): add -W to yarn add when in yarn workspaces root #121

merged 3 commits into from
Jan 6, 2021

Conversation

HendrikThePendric
Copy link
Contributor

The command d2-utils-cypress install would not complete successfully in a yarn workspaces repo due to the missing -W flag. This fixes it, but I wonder if there would be a more elegant way to do this...

@HendrikThePendric HendrikThePendric requested review from amcgee and a user January 4, 2021 14:29
@HendrikThePendric HendrikThePendric self-assigned this Jan 4, 2021
amcgee
amcgee requested changes Jan 4, 2021
View reviewed changes
Copy link
Member

@amcgee amcgee left a comment

Choose a reason for hiding this comment

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

@HendrikThePendric I think this will throw if package.json doesn't exist or isn't valid JSON, so if going with this approach it at least needs a try/catch

@HendrikThePendric
Copy link
Contributor Author

I did consider that, but thought I would just let the error throw in any case. If there is no package.json or it contains invalid json, this script failing is the least of the repo's worries right?

@amcgee
Copy link
Member

amcgee commented Jan 4, 2021

It could fail just because it's being run in a directory that doesn't happen to be a package root - in that case it would be much nicer to say "install must be run from the root of an npm package, but no package.json was found" rather than simply an IO failure with a nasty stack trace.

@amcgee
Copy link
Member

amcgee commented Jan 4, 2021

Technically we should find-up to the nearest package root anyway, rather than assuming the CWD is the root

@HendrikThePendric HendrikThePendric changed the base branch from master to next January 5, 2021 15:28
amcgee
amcgee approved these changes Jan 6, 2021
View reviewed changes
Copy link
Member

@amcgee amcgee left a comment

Choose a reason for hiding this comment

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

LGTM - suggested a minor update to the message

packages/cli/src/commands/install/ensureDependency.js Outdated Show resolved Hide resolved
@HendrikThePendric HendrikThePendric changed the title fix(commands): add -W to yarn install when in yarn workspaces root fix(commands): add -W to yarn add when in yarn workspaces root Jan 6, 2021
@HendrikThePendric HendrikThePendric changed the title fix(commands): add -W to yarn add when in yarn workspaces root fix(commands): add -W to yarn add when in yarn workspaces root Jan 6, 2021
@HendrikThePendric HendrikThePendric merged commit 6c5a369 into next Jan 6, 2021
@HendrikThePendric HendrikThePendric deleted the CLI-21-fix-install-command-in-yarn-workspaces-repos branch January 6, 2021 11:43
dhis2-bot added a commit that referenced this pull request Jan 6, 2021
@dhis2-bot
chore(release): cut 5.0.1 [skip ci]
a8589aa 
## [5.0.1](v5.0.0...v5.0.1) (2021-01-06)

### Bug Fixes

* **commands:** add `-W` to `yarn add` when in yarn workspaces root ([#121](#121)) ([6c5a369](6c5a369))
* **commands:** adjust generated code changes ([#124](#124)) ([2c6de92](2c6de92))
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 5.0.1 🎉

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
Reviewers

@amcgee amcgee amcgee approved these changes

Assignees

@HendrikThePendric HendrikThePendric

Labels
released on @next
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@HendrikThePendric @amcgee @dhis2-bot