-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(api): addition of a new uninstall command #776
Conversation
Co-authored-by: Kanad Gupta <[email protected]>
Co-authored-by: Kanad Gupta <[email protected]>
Co-authored-by: Kanad Gupta <[email protected]>
@@ -45,6 +45,12 @@ | |||
"sha512-ld+djZk8uRWmzXC+JYla1PTBScg0NjP/8x9vOOKRW+DuJ3NNMRjrpfbY7T77Jgnc87dZZsU49robbQfYe3ukug==" | |||
] | |||
}, | |||
"language": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping track of what language we generated the SDK for will allow us to know how to uninstall it from that languages package manager. And because this didn't exist before this we'll default to js
everywhere we access this.
@@ -30,6 +30,7 @@ cmd | |||
logger(`package name (${chalk.red('private')}): ${chalk.grey(`@api/${api.identifier}`)}`); | |||
} | |||
|
|||
logger(`language: ${chalk.grey(api.language || SupportedLanguages.JS)}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -226,30 +282,17 @@ export default class Storage { | |||
} | |||
|
|||
/** | |||
* @example <caption>Storage directory structure</caption> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed this because I don't want to have to maintain it alongside the same tree in docs/how-it-works.md
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pretty much only docs feedback below but this works great in my testing! if we're uninstalling the last remaining SDK, should we also remove the top-level .api
directory and all of its contents or nah?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry for all the feedback in this file — i definitely plan on doing a full comb through our docs before we release v7
const entry = Storage.getFromLockfile(identifier); | ||
if (!entry) { | ||
logger( | ||
`You do not appear to have ${identifier} installed. You can run \`npx api list\` to see what SDKs are present.`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is great 🫶🏽
Co-authored-by: Kanad Gupta <[email protected]>
Co-authored-by: Kanad Gupta <[email protected]>
Co-authored-by: Kanad Gupta <[email protected]>
Co-authored-by: Kanad Gupta <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one more tiny thing — otherwise LGTM!
Co-authored-by: Kanad Gupta <[email protected]>
🧰 Changes
This creates a new
uninstall
command that folks can use to uninstall codegen'd SDKs from their codebase. Not only will it runnpm uninstall @api/<identifier>
it will also delete the generated SDK out of the.api/
directory. Here's what it all looks like.