Login issues #487
Login issues #487
Conversation
Issue rustwasm#484. PR392 inadvertantly replaced the `login` interactive process spawner with `child::run`, which is hard-coded to buffer stdout/stderr. This caused `login` to become essentially unusable; the user could no longer see interactive input prompts or error messages displayed by `npm adduser`. The code was not directly reverted because the previous version: 1. Returned Error instead of failure::Error. (Updated to use `bail!`, which is consistent with `publish`.) 2. Displayed all stderr only upon exit, rather than interactively displaying it. This led to repeated interactive prompts without informing the user why. (Updated to use `status()` which inherits stdin/stdout/stderr by default.) 3. Did not provide logging. (Now duplicates the logging in `child::run`.)
All arguments were passed to `arg` inside one string when building a `Command`. However, using the `arg` function, "only one argument can be passed per use." This caused all arguments accidentally to be appended to the registry URL. For example: After a successful login with a provided `--auth_type`, the success message incorrectly displayed: "Logged in as asf on https://registry.npmjs.org/%20--auth_type=Basic." The space (%20 in hex) was caused by adding a fixed space before each additional argument. This commit pushes all arguments onto a `Vec<String>`. Then, the `args` function adds the arguments separately to the command. This removes the need to prepend spaces to each argument. Alternatively, `arg` could have been used throughout to build the command argument-by-argument. However, using `args` partitions the code more neatly into two distinct sections.
|
hi @danwilhelm ! thanks so much for taking this on! looks good to me- want to test locally before approving. tagging in @fitzgen due to the mentioned limitations of the child process abstraction- curious what his thoughts are! |
|
@fitzgen It would be nice to keep all of the process code in child.rs. So alternatively, I could perhaps add a Note the solution I proposed here does not log |
There was a problem hiding this comment.
This LGTM!
In general, I hate interactive CLI tools, and would prefer that if they need more info, just exit non-zero and say "use --whatever flag to specify this info we are missing", but since this is npm doing this, and we are wrapping npm here, we don't really have much choice.
Thanks for the PR @danwilhelm!
I'm not sure what If I'm missing something, please file a follow up issue with suggestions and we can take the discussion there! :) |
|
I agree with both of you, which is why I implemented it as-is! Indeed, a new function |
login.loginarguments being accidentally merged together when spawningnpm adduser.See the text of each commit for more details.
rustfmtinstalledcargo fmton the code base before submitting