Fixed an issue with parsing --json output when publishing
#676
Conversation
🦋 Changeset detectedLatest commit: 8f7d275 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Andarist
force-pushed
the
fix/stdout-json-parsing
branch
from
6b02080 to
cd1dae2
Compare
| json.error.summary, | ||
| json.error.detail ? "\n" + json.error.detail : "" | ||
| ); | ||
| error(stderr); |
There was a problem hiding this comment.
Added this as suggested here
For the moment I've refrained from printing stdout too because it contains the output of all lifecycle scripts and it feels like this could get really, really, verbose.
A somewhat weird effect is also that with npm7+ this will contain at the end the JSON mentioned by the error above this line - but I guess it's not worth trying to remove that.
| export const getLastJsonObjectFromString = (str: string) => { | ||
| str = str.replace(/[^}]*$/, ""); | ||
|
|
||
| while (str) { | ||
| str = str.replace(/[^{]*/, ""); | ||
|
|
||
| try { | ||
| return JSON.parse(str); | ||
| } catch (err) { | ||
| // move past the potentially leading `{` so the regexp in the loop can try to match for the next `{` | ||
| str = str.slice(1); | ||
| } | ||
| } | ||
| return null; | ||
| }; |
There was a problem hiding this comment.
How would this handle the thing below and similar things with { and } in strings?
{"blah": "{"}There was a problem hiding this comment.
the given example is not even that problematic - we try to parse from { till the end, and since this is a valid JSON it just works
if we consider an input like:
"{" {"blah": "{"}
then it also works because it will fail on the quoted one and retry on the second one (and succeed there)
The idea behind this helper is a little bit indirect but the gist is that even if we catch { somewhere, wherever really, be it in a quoted string, be it orphaned, be it part of some other JSON, then we try to parse from it and we simply fail because all of those cases can't be parsed as a single JSON. So we continue and we only parse successfully when we get to the last JSON in the string.
There is also an assumption that we can't have some "invalid" characters after that last JSON - this helper isn't working correctly if we consider such scenario (so {"a": "b"}randomtext won't result in a "last json object from string"). Such a scenario has no application for us - because the JSON that we are after should really always be the last thing in the stdout/stderr
There was a problem hiding this comment.
There is also an assumption that we can't have some "invalid" characters after that last JSON - this helper isn't working correctly if we consider such scenario (so {"a": "b"}randomtext won't result in a "last json object from string"). Such a scenario has no application for us - because the JSON that we are after should really always be the last thing in the stdout/stderr
Running npm publish(npm@8.1.0) in a non-tty env, I get what's shown below. How is it working if it breaks with "invalid" characters after the last json given there are "invalid" characters after the json below?
npm ERR! code EOTP
npm ERR! This operation requires a one-time password from your authenticator.
npm ERR! You can provide a one-time password by passing --otp=<code> to the command you ran.
npm ERR! If you already provided a one-time password then it is likely that you either typoed
npm ERR! it, or it timed out. Please try again.
{
"error": {
"code": "EOTP",
"summary": "This operation requires a one-time password from your authenticator.",
"detail": "You can provide a one-time password by passing --otp=<code> to the command you ran.\nIf you already provided a one-time password then it is likely that you either typoed\nit, or it timed out. Please try again."
}
}
npm ERR! A complete log of this run can be found in:
npm ERR! /Users/mitchell/.npm/_logs/2021-11-25T22_31_42_856Z-debug.log
There was a problem hiding this comment.
Sorry, I forgot that I've actually put this at the beginning of this function: str = str.replace(/[^}]*$/, "");.
So some invalid characters after the last JSON are allowed, but it would break on stuff like:
{"some": "json"}
npm ERR! /some/weird/path/file}name.log
This seems acceptable as it's a really bizarre case - we can always refine this logic later when the need arises. Unless you have any idea how to improve this logic in an easy and quick way to handle more scenarios?
| getLastJsonObjectFromString(stderr.toString()) || | ||
| getLastJsonObjectFromString(stdout.toString()); | ||
|
|
||
| if (json.error) { |
There was a problem hiding this comment.
| if (json.error) { | |
| if (json?.error) { |
There was a problem hiding this comment.
Do we have any scenario in which this would be currently needed? Or is it intended to be as an extra precaution?
There was a problem hiding this comment.
Don't see anything that would require it, just an extra precaution because getting Uncaught TypeError: Cannot read properties of null (reading 'error') would be pretty annoying.
There was a problem hiding this comment.
Ok, I've adjusted the code as suggested and made sure that even if it's missing then we still return { published: false } (as this is still within the if (code !== 0) block)
|
👏🏼 Will the changeset action (https://github.com/changesets/action) also receive this update? |
|
@peschee Sorry, I've missed your comment here - what do you mean? Changesets Action is just using the installed Changesets CLI - and AFAIR it doesn't make direct calls to the npm CLI. So this shouldn't quite affect it directly, unless I'm missing something. |
This work has been moved out of #674
closes #567
closes #397