Skip to content

fix: use npmFetch() instead of npmFetch.json() for team destroy command#6161

Merged
wraithgar merged 6 commits intonpm:latestfrom
heisantosh:no-fetchjson-for-team-destroy
Feb 14, 2023
Merged

fix: use npmFetch() instead of npmFetch.json() for team destroy command#6161
wraithgar merged 6 commits intonpm:latestfrom
heisantosh:no-fetchjson-for-team-destroy

Conversation

@heisantosh
Copy link
Copy Markdown
Contributor

@heisantosh heisantosh commented Feb 13, 2023

The registry returns a HTTP 204 status and an empty body on success for the npm team destroy <org>:<team> command. Using npmFetch.json() makes the CLI error out on completion even though the action was completed successfully in the registry.

Using npmFetch() will not attempt to parse the response body as JSON.

The registry returns a 204 status and an empty body on success for the
team destroy command. Using npmFetch.json() makes the CLI error out on
completion even though the action was completed successfully in the
registry.
@heisantosh heisantosh requested a review from a team as a code owner February 13, 2023 12:37
@heisantosh heisantosh requested review from lukekarrys and removed request for a team February 13, 2023 12:37
@heisantosh heisantosh marked this pull request as draft February 13, 2023 12:59
@heisantosh
Copy link
Copy Markdown
Contributor Author

Looks like I have to publish a new version of libnpmteam and also update the test cases for the team destroy command.

@heisantosh heisantosh force-pushed the no-fetchjson-for-team-destroy branch from 68f511e to 65475ce Compare February 13, 2023 13:29
Return a value of empty JSON {} object to have a known response for the
empty response body. Otherwise the npmFetch returns the Body object from
minipass package.
@heisantosh heisantosh marked this pull request as ready for review February 13, 2023 14:22
@wraithgar
Copy link
Copy Markdown
Contributor

It'd be good if we caught the correct behavior in the tests. Can you remove the {} from the 204 responses in the test file test/index.js?

Comment thread workspaces/libnpmteam/lib/index.js Outdated
Comment thread workspaces/libnpmteam/lib/index.js Outdated
@wraithgar
Copy link
Copy Markdown
Contributor

Fixed PR to use same delete behavior as libnpmaccess

@wraithgar wraithgar merged commit 11e6cc9 into npm:latest Feb 14, 2023
@github-actions github-actions Bot mentioned this pull request Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants