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

RFC: introduce volta list and deprecate volta current #34

Merged
merged 78 commits into from Jul 15, 2020
Merged

Conversation

@chriskrycho
Copy link
Contributor

@chriskrycho chriskrycho commented May 15, 2019

Rendered RFC text. See earlier discussion at volta-cli/volta#349.

Add an informational command, volta list, replacing the volta current command and expanding on its capabilities. The list command will allow users to see their default and project-specific toolchains, and will support both human-friendly output and a tool-friendly output.

text/0000-informational-commands.md Outdated Show resolved Hide resolved
Loading
text/0000-informational-commands.md Show resolved Hide resolved
Loading
text/0000-informational-commands.md Outdated Show resolved Hide resolved
Loading
```sh
$ volta list
runtime node@v10.15.3 (default)
packager yarn@v1.12.3 (default)
Copy link
Contributor Author

@chriskrycho chriskrycho May 16, 2019

Choose a reason for hiding this comment

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

I'm unsure what we should actually output here: if the user has install-ed Yarn, presumably we should print it. Should we also always print the built-in npm? I currently lean toward yes; that would then extend to the human format as well. Thoughts?

Loading

Copy link
Contributor

@mikrostew mikrostew May 30, 2019

Choose a reason for hiding this comment

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

I think printing the built-in npm is a good idea, along with what that version is

Loading

@chriskrycho chriskrycho force-pushed the info-commands branch 4 times, most recently from 03237ad to 751a726 May 29, 2019
@chriskrycho chriskrycho marked this pull request as ready for review May 29, 2019
chriskrycho and others added 22 commits May 29, 2019
- identify whether the item is a runtime, a packager, or a tool
- update the list to match the example user configuration
- use the correct invocation for plain, `--print=plain`
- show both bare subcommand and `--all` for plain output
Co-Authored-By: Robert Jackson <me@rwjblue.com>
Copy link
Contributor

@mikrostew mikrostew left a comment

Awesome! Just a few comments on things I saw reading through this RFC.

(also, great use of <details> 😄 )

Loading

- to runtimes: `volta list node`
- to packagers: `volta list yarn` (and, when custom installations of `npm` are supported, `volta list npm`)
- to a specific package: `volta list <package name>`
- to a specific tool: `volta list <tool name>`
Copy link
Contributor

@mikrostew mikrostew May 30, 2019

Choose a reason for hiding this comment

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

These two options don't match the format below in "Details", which is:
volta list package <package>
volta list tool <tool>

Loading

Copy link
Contributor Author

@chriskrycho chriskrycho May 30, 2019

Choose a reason for hiding this comment

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

Ah, good catch. Updated for consistency, though depending on how we sort out the related question I may have to update it the other direction. 😆

Loading

text/0000-informational-commands.md Outdated Show resolved Hide resolved
Loading
text/0000-informational-commands.md Show resolved Hide resolved
Loading
```sh
$ volta list
runtime node@v10.15.3 (default)
packager yarn@v1.12.3 (default)
Copy link
Contributor

@mikrostew mikrostew May 30, 2019

Choose a reason for hiding this comment

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

I think printing the built-in npm is a good idea, along with what that version is

Loading

text/0000-informational-commands.md Outdated Show resolved Hide resolved
Loading
For the TypeScript config specified in the canonical example:

```sh
volta list package typescript --human
Copy link
Contributor

@mikrostew mikrostew May 30, 2019

Choose a reason for hiding this comment

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

Slight discrepancy here between volta list <package> and volta list package typescript.

This will probably need to be volta list package <package> and volta list tool <tool> to be able to disambiguate when packages and tools have the same names (like the create-react-app example).

Loading

Copy link
Contributor Author

@chriskrycho chriskrycho May 30, 2019

Choose a reason for hiding this comment

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

This is actually the reason why I had volta list package <package> and volta list tool <tool> in an earlier draft of the RFC. I'm back-and-forth on it, because in the case you identified, it's the same output either way. In a case where there's more than one binary, but one does match the package name, I don't think it harms anything to default to the package in that case? 🤔 But I'm very willing to be persuaded here—I just found myself leaning slightly in the direction of brevity!

Loading

Copy link
Contributor

@mikrostew mikrostew May 30, 2019

Choose a reason for hiding this comment

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

After thinking about this more, I like the volta list <package-or-tool> approach, where it defaults to the package. It's more compact to type, and in cases where the binary and the package are the same name, you get more information.

Loading

- all available package binaries?
- a subset of package binaries once the number crosses a threshold, with instructions about how to see all?
- no package binaries?
Copy link
Contributor

@mikrostew mikrostew May 30, 2019

Choose a reason for hiding this comment

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

Currently the only way to create a shim for a binary is to use volta install <package>, which installs the package and shims any of the installed binaries. Then the shim will intercept any invocation inside of a project to detect if it is a package binary.

Since this is already displaying the available binaries (from installed packages), the available package binaries will be a subset of that, and could probably be displayed with some indication that they are coming from the current project.

Loading

Copy link
Contributor Author

@chriskrycho chriskrycho May 30, 2019

Choose a reason for hiding this comment

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

This plus your suggestion below make a ton of sense to me; I'll update the design accordingly and at least the initial implementation will work that way.

Loading

- a subset of package binaries once the number crosses a threshold, with instructions about how to see all?
- no package binaries?
And how should that be presented in `plain` vs. `human` mode? Should there be any differences between `plain` and `human` for the bare command (as in the current design)?
Copy link
Contributor

@mikrostew mikrostew May 30, 2019

Choose a reason for hiding this comment

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

My first thought at this would be the addition of a single line (if for instance the project uses ember):

$ volta list --human
⚡️ Currently active tools:
    Node: v8.16.0 (current @ ~/node-only/package.json)
    Yarn: v1.12.3 (default)
    Tool binaries available:
        create-react-app, tsc, tsserver
        ember (current @ ~/node-only/package.json)

Loading

Copy link
Contributor Author

@chriskrycho chriskrycho May 30, 2019

Choose a reason for hiding this comment

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

I like that design a lot. Should the previous line then include (defaults) to distinguish them? 🤔

Loading

Copy link
Contributor

@mikrostew mikrostew May 30, 2019

Choose a reason for hiding this comment

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

I think having (defaults) for the first is a great idea to make the distinction obvious

Loading

Copy link
Contributor

@charlespierce charlespierce left a comment

This is awesome and incredibly detailed! I don't have any substantive comments on the command itself, just a few comments around the edges.

Loading


## Use another command name

As noted in [**Why `list`?**][why-list], options for this subcommand name include `list`, `ls`, `tools`, `toolchain`, `info`, and `current`. (For discussion of shorthands, see the next section.) Given the considerable [prior art][prior-art] using `list`, however, it seems the best option in terms of what users will expect.
Copy link
Contributor

@charlespierce charlespierce May 31, 2019

Choose a reason for hiding this comment

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

I like the idea (first mentioned in Discord I believe) of having ls be a full alias of list, just for ergonomic reasons. It doesn't have to be documented or shown anywhere, just as a bonus for usability & potential discoverability of the command.

Loading


## Deprecating `volta current`

Since `volta list` subsumes (and substantially extends upon) the functionality of `volta current`, `volta current` should be deprecated when `volta list` is implemented. The deprecation should include a warning that the command will be removed in a future version and information about how to use `volta list --current <tool>` as a replacement.
Copy link
Contributor

@charlespierce charlespierce May 31, 2019

Choose a reason for hiding this comment

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

I like this approach a lot, it allows us to introduce the command and the deprecation warning without a breaking change, and then the next time we have a breaking change we can take that opportunity to remove the current command (that I suspect no one is using anyway given its limited functionality).

Loading

- Should the command accept flags to narrow the search instead of a positional argument, e.g. `volta list --node`, `volta list --yarn`, `volta list --package=typescript`, etc.?
- How should the built-in version of `npm` be displayed to the user? Options include:
- associated with the runtime it ships with, e.g. `v12.2.0 (with npm v6.4.1)`
Copy link
Contributor

@charlespierce charlespierce May 31, 2019

Choose a reason for hiding this comment

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

This is how we currently show it when you run e.g. volta install node, so if possible it would be good to align with that. Also, until we implement the volta install npm logic, listing npm as a separate packager seems like it has the potential for confusion, implying that the user can manage npm themselves.

Loading

@chriskrycho chriskrycho force-pushed the info-commands branch 2 times, most recently from 04a3e33 to b629a9a Jun 21, 2019
chriskrycho added 2 commits Jul 31, 2019
- Go back to using `(default|current @ <path>|fetched)` as the design
  for indicating the origin of a given item, but with the
  simplifications made for packages in the previous commit.
- Add an example for local package overrides.
@mikrostew
Copy link
Contributor

@mikrostew mikrostew commented Jun 10, 2020

Now that volta-cli/volta#461, volta-cli/volta#701, and volta-cli/volta#697 have landed, is this RFC ready to be merged? Should we wait for volta-cli/volta#659?

Loading

@charlespierce charlespierce changed the base branch from master to main Jun 15, 2020
@chriskrycho
Copy link
Contributor Author

@chriskrycho chriskrycho commented Jun 22, 2020

I think this is quite ready to merge. We may continue to add more output formats and to iterate on the design as we find pain points, but the RFC has served its purpose and is fully implemented given our existing constraints from where I stand!

Loading

@charlespierce charlespierce merged commit cbb8525 into main Jul 15, 2020
@charlespierce charlespierce deleted the info-commands branch Jul 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants