Rewrite publish logic #132
Conversation
|
Posting this feedback from the Discord thread, in case it gets lost otherwise:
Working on Sadly it's seeming like we might not have any unit tests for this? |
|
Speaking of which, the Discord thread: https://discord.com/channels/992103415163396136/1241217301529886853 And the start of the recent round of discussion around this topic, preceding the formal Discord thread about it by a bit: https://discord.com/channels/992103415163396136/992109539346370661/1240836443299905606 |
src/publish.js
Outdated
| let originalName; | ||
| if (version?.length <= 0) { version = 'patch'; } | ||
| if (rename?.length > 0) { originalName = pack.name; } | ||
| if (version?.length === 0 && tag?.length === 0 && rename?.length === 0) { |
There was a problem hiding this comment.
I think it would be a benefit to know this is somehow still ending up undefined (despite the normalization we're doing) sooner, due to undefined.length giving an error Uncaught TypeError: Cannot read properties of undefined (reading 'length').
Rather than end up silently doing some tricky comparisons of undefined vs 0 (undefined === 0 specifically, which would be false, seeming to imply that the length is greater than 1 as it's "not 0", which I think puts us on the wrong code flow path in the if, right?)
| if (version?.length === 0 && tag?.length === 0 && rename?.length === 0) { | |
| if (version.length === 0 && tag.length === 0 && rename.length === 0) { |
Sort of theoretical, since it shouldn't be possible.
But also, I like to avoid more complicated syntax if it's not needed, for ease of reading.
Also: Given the head-scratcher this issue was to solve, correctness and surfacing error messages in as straight-forward a way as possible is kind of my top priority, rather than avoiding errors in this case. Unless we want to try/catch and print the error and exit gracefully, but... I don't think that helps anything anyway in this case.
tl;dr: Hitting a simple error message would be better than not hitting it, in this case, I think? So, optional chaining not a benefit here?
Not a blocker, since it should all be hypothetical with all the normalizing done in this PR. But the goose-chase tracking down strange behavior makes me a tad paranoid, not gonna lie.
There was a problem hiding this comment.
Sorry for all the nitpicking, just inclined to make this code very tidy if possible after what had to be thought through to get here.
There was a problem hiding this comment.
Note: If moving away from optional chaining for the varName.length checks, there are three more in the diff for this PR, I just didn't individually comment them all.
There was a problem hiding this comment.
P.S. I suppose these instances of optional chaining mirror what was there before this PR, but while we're addressing tiny subtleties of behavior that were unintuitive, I think it's worth deciding whether to move away from the optional chaining or not.
| if (code === 0) { | ||
| if (code !== 0) { |
There was a problem hiding this comment.
Just commentary, no action needed:
Kind of odd to see that this check was coded wrong from before this PR, I believe it was inadvertently changed as part of the utterly massive promisification PRs, from looking at the blame history.
This change seems legit. All other calls to logFailure() are when code !== 0, as in error code rather than clean exit code. So yeah, seems legit IMO.
There was a problem hiding this comment.
Was surprised to see this one broken for so long tbh
There was a problem hiding this comment.
Approving, since this fixes the various intended use-cases, such as ppm publish patch for a first publish, while keeping --tag working, and fixing --rename which may have been broken for some time on master branch.
Might be a good idea either in this PR to do a follow-up to handle invalid combos of CLI args, such as doing patch, --tag, and --rename all at once? But I'm not sure that's actually invalid or if someone could intentionally do something useful with combining them? So, this is optional to address, IMO. We can tell people to do just one at a time without having the code print an error message about it for now?
If you do limit to only one of them being set at a time, that could be reasonable, but honestly I think it's good enough for now without that as well.
|
Thanks for the approval, gonna go ahead and merge this one for now, then we just gotta get it updated in pulsar sometime soon |
After numerous issues and reports and pain points of this publish logic, we hit a point after enabling strict versioning on the backend to discover that the original logic completely fails now.
I'll try to update this with more details with more time, but for now, here's an attempt that should resolve our woes.