Skip to content

Conversation

wraithgar
Copy link
Member

@wraithgar wraithgar commented May 7, 2022

It was querying whoami once for every package you starred/unstarred, and
incorrectly trying to determine if you weren't logged in. In fact the
function throws a descriptive message if you're not logged in already.
The whoami check was also racing with the fetch of the packument for
each package you were starring/unstarring meaning you could also get a
random 401 for a private package instead of the 'you need to log in'
message.

unstar was setting an undocumented config item to get the
shared code to unstar. The command already has a name attribute that
tells us what action we are doing so we can just use that.

Finally, the duplicated (and differing) params between the two commands
were consolidated.

@wraithgar wraithgar requested a review from a team as a code owner May 7, 2022 16:12
@wraithgar wraithgar force-pushed the gar/star-cleanup branch 2 times, most recently from 0b2a933 to 146b564 Compare May 7, 2022 16:15
It was querying whoami once for every package you starred/unstarred, and
incorrectly trying to determine if you weren't logged in.  In fact the
function throws a descriptive message if you're not logged in already.
The whoami check was also racing with the fetch of the packument for
each package you were starring/unstarring meaning you could also get a
random 401 for a private package instead of the 'you need to log in'
message.

unstar was setting an undocumented config item to get the
shared code to unstar.  The command already has a name attribute that
tells us what action we are doing so we can just use that.

Finally, the duplicated (and differing) params between the two commands
were consolidated.
@wraithgar wraithgar force-pushed the gar/star-cleanup branch from 146b564 to a97bf8f Compare May 7, 2022 16:22
@npm-robot
Copy link
Contributor

npm-robot commented May 7, 2022

no statistically significant performance changes detected

timing results
app-large clean lock-only cache-only cache-only
peer-deps
modules-only no-lock no-cache no-modules no-clean no-clean
audit
npm@8 52.222 ±1.05 33.812 ±0.00 36.502 ±23.79 21.459 ±0.64 3.009 ±0.00 3.016 ±0.07 2.475 ±0.20 12.289 ±0.01 2.421 ±0.04 3.466 ±0.00
#4868 55.368 ±0.65 33.046 ±0.07 33.093 ±19.68 21.705 ±1.04 3.013 ±0.00 2.988 ±0.00 2.405 ±0.04 12.266 ±0.01 2.394 ±0.03 3.599 ±0.26
app-medium clean lock-only cache-only cache-only
peer-deps
modules-only no-lock no-cache no-modules no-clean no-clean
audit
npm@8 39.894 ±0.83 25.055 ±0.06 14.454 ±0.14 14.651 ±0.24 2.771 ±0.04 2.795 ±0.05 2.434 ±0.01 8.977 ±0.16 2.274 ±0.00 3.086 ±0.02
#4868 40.108 ±0.92 25.695 ±0.16 13.727 ±0.12 14.645 ±0.28 2.763 ±0.02 2.788 ±0.02 2.489 ±0.03 8.954 ±0.07 2.263 ±0.01 3.240 ±0.14
@wraithgar wraithgar merged commit 38cf29a into latest May 9, 2022
@wraithgar wraithgar deleted the gar/star-cleanup branch May 9, 2022 14:34
@wraithgar wraithgar mentioned this pull request May 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants