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

Fall back to default Yarn if project Yarn is not set #470

Merged
merged 2 commits into from
Jun 27, 2019

Conversation

charlespierce
Copy link
Contributor

@charlespierce charlespierce commented Jun 12, 2019

Closes #436

Info

  • Currently, if we have a default yarn set, but the user runs yarn within a project that doesn't have yarn specified, then we throw an error and require the user to run volta pin yarn to continue.
  • This is a poor UX, as the user has already run volta install yarn, so we do have a Yarn version available, we are just not using it.
  • Additionally, with the command passthrough and other UX improvements, we have taken the approach of setting Volta up to support doing things the "safe" way (e.g. if you use yarn in your project, you should specify it there), but not require that they are done that way.
  • To align with that goal, we should fall back to the default yarn, even in a project that doesn't have yarn specified.

Changes

  • Added an additional platform::Source: ProjectNodeUserYarn, that represents a merged platform.
  • Implemented the fall-back logic in session::current_platform, so that the fallback is available to any tool (as running other tools could result in running yarn, e.g. ember new --yarn).
  • Updated the tool implementations to account for the new source.

Tested

  • Added new acceptance tests to cover the new behavior.

@charlespierce charlespierce marked this pull request as ready for review June 18, 2019 22:24
@charlespierce charlespierce changed the base branch from master to implement-rfc-34-list-command June 18, 2019 22:27
@charlespierce charlespierce changed the base branch from implement-rfc-34-list-command to master June 18, 2019 22:27
@chriskrycho
Copy link
Contributor

Is this ready to go? If so, I’ll review it and we can merge it! Sorry for the delay; I missed seeing it was open!

@charlespierce
Copy link
Contributor Author

Yeah, sorry I think I might have forgotten to mention it was out of draft state. Thanks!

@rwjblue
Copy link
Contributor

rwjblue commented Jun 27, 2019

FWIW, I think GH needs to get better about notifications here. I don't think they send another notification when going from "Draft" to "Ready to review".

@chriskrycho
Copy link
Contributor

CONFIRM, @rwjblue. I'm reviewing now!

Copy link
Contributor

@chriskrycho chriskrycho left a comment

Choose a reason for hiding this comment

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

This is a solid fix and I think we should ship this so users stop hitting this issue… but I do have a question about whether this is exactly the right overall approach; it seems to me that between this and some of the stuff I've been doing in the volta list implementation, we might have motivation to do some more substantial restructuring around this idea of the source and its relationship to the platform.

crates/volta-core/src/platform/sourced.rs Outdated Show resolved Hide resolved
crates/volta-core/src/platform/sourced.rs Show resolved Hide resolved
@charlespierce
Copy link
Contributor Author

Created https://github.com/volta-cli/volta/projects/6#card-23308438 and https://github.com/volta-cli/volta/projects/6#card-23308348 to track possible future refactors brought up in PR Review.

@charlespierce charlespierce merged commit 89d418c into volta-cli:master Jun 27, 2019
@charlespierce charlespierce deleted the user_yarn branch June 27, 2019 16:44
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.

Volta error: No Yarn version found in this project.
3 participants