volta-cli / rfcs

RFCs for changes to Volta
Other
17 stars 14 forks source link

Command for overriding platform: `volta run` #39

Closed charlespierce closed 4 years ago

charlespierce commented 5 years ago

Add a volta run command, allowing users to execute tools with an override on the detected platform.

Rendered

charlespierce commented 5 years ago

@rwjblue Thanks for the feedback, that's great and super helpful! To answer your other questions:

  1. Environment variables can be specified in the usual way VARIABLE=value volta run ... on Unix, though I could see the benefit of having a --env VARIABLE=value flag as well, to unify the UX and support those on Windows (which doesn't have an easy shorthand for setting one-off environment variables in all shells).

  2. We still need to parse the first part of the command to figure out which shim behavior to use (in the same way that we parse the command name when running node or ember). The arguments, however, are just in an array and will be left untouched.

  3. See the answer for 1, you can / should specify them before volta run

  4. We don't have to specialize --node and --yarn, we could probably do something like --with node@8. My initial concern when doing that was how would we handle if someone included the same tool multiple times (e.g. --with node@8 --with node@10, but in those cases we can probably error out early and tell them to only set each value once.

  5. I like the idea of supporting a JSON config, though specifying it directly on the command line seems tricky, since in order for it to be detected as a single argument, it will need to be quoted. That combined with necessary quotes around the property names could get unwieldy for the user. But supporting a JSON config file should be relatively straightforward.

    • One thing to note for that, however, is that currently the JSON objects that we deal with represent fully-resolved versions (i.e. You can't have "node": "8" in your package.json). On the command-line, we would want to support partially-specified versions, so that will be a slight departure in this use-case.

I'll think on some of these issues a bit more and then look to fill out some more details :+1:

rwjblue commented 5 years ago

which doesn't have an easy shorthand for setting one-off environment variables in all shells

Great point!

We still need to parse the first part of the command to figure out which shim behavior to use (in the same way that we parse the command name when running node or ember).

From initially reading this I would assume that <command> is literally what I would have typed on the command line to run the command. This lead me to believe that the following would work (and was being proposed by this RFC):

volta run --node 10 EMBER_ENV=production ember build

However, I don't think thats correct (at least it doesn't seem like thats what you were saying in your comment above). Can you confirm/clarify?

charlespierce commented 5 years ago

To clarify, that won't work with the current proposal. Ultimately, we have to do some parsing of the passed-in command, in order to figure out which tool is being executed and run our unique logic for that tool. Since we can't reasonably reproduce the command-parse logic of every shell, we need the command to start with the name of the tool (in your example, we would see EMBER_ENV=production as the "tool name" and then fail to find that "tool").

I wonder if there's a better way we can explain / clarify how that works (and maybe having the --env option would reduce the chance of a user accidentally doing that.

rwjblue commented 5 years ago

Since we can't reasonably reproduce the command-parse logic of every shell, we need the command to start with the name of the tool

Yeah, I definitely see the issue you are describing (this is exactly the reason I asked about parsing above). I am definitely liking the idea of passing the env vars via --env to volta run (I've been looking for a convenient cross platform way of doing this sort of thing for my CI runs in various projects). If we do that and essentially "lint" the string after known volta arguments to emit a nice warning when you try what I had pasted (volta run --node 8 EMBER_ENV=foo ember b) it may be good enough...


Oh, also this question:

Have we considered supporting environment variables (in addition to or instead of volta run)?

Was talking about VOLTA_NODE=8 ember b (not like the third question which was about passing environment variables to the invoked command).

charlespierce commented 5 years ago

Added a bunch more to address many of the comments raised by @rwjblue