-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
improve link command #10257
improve link command #10257
Conversation
|
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.
Thanks for adding the CLI so prompt
ly! Left comments mostly on API signature.
// TODO(fks): Actually listen for project creation before continuing | ||
// This is just a dumb spinner that roughly matches database creation time. | ||
const spinner = ora('Creating new project...'); | ||
await new Promise((r) => setTimeout(r, 4000)); |
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.
"First make it work, then make it good."
~ Confucius, probably
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.
When this is addressed, it would be great to provide a link to your project dashboard!
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.
oh nice, that's a great call. Just refactored the code a bit to make this even easier in the future
body: JSON.stringify({ name, region }), | ||
}); | ||
if (!response.ok) { | ||
console.error(`Failed to create project: ${response.status} ${response.statusText}`); |
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.
Can we standardize to the { success: boolean, value: JSON }
format for reading errors? I worry statusText
could log errors that aren't human-readable
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.
ah good catch, I think that already is the response format and this is just checking for the case where our entire API fails and you just get back an unhandled 500. But, we should also have proper handling for the handled error case.
const linkUrl = new URL(getAstroStudioUrl() + '/api/cli/projects.list'); | ||
const response = await fetch(linkUrl, { | ||
method: 'POST', | ||
headers: { | ||
Authorization: `Bearer ${await getSessionIdFromFile()}`, | ||
'Content-Type': 'application/json', | ||
}, | ||
body: JSON.stringify(body), | ||
body: JSON.stringify({}), |
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.
nit: should this be a GET? Seems odd to post an empty body to get a list of projects
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.
Because we're already maintaining TRPC, we follow an RPC-like approach to this API as well, which effectively says that request method doesn't matter (so POST is okay even if you're just getting data).
|
||
export async function promptBegin(): Promise<void> { | ||
// Get the current working directory relative to the user's home directory | ||
const prettyCwd = process.cwd().replace(homedir(), '~'); |
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.
Please work on windows please work on windows please work on
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.
it should... right? homedir()
is platform agnostic so I think this should work. We'll bug @itsMapleLeaf after merging to test :)
c154db6
to
c912dd5
Compare
c912dd5
to
34097bb
Compare
* main: (327 commits) [ci] format fix(node): listen on 0.0.0.0 if server.host is set to true (withastro#10282) [ci] format fix(dev): cosider `base` when special-casing `/_image` (withastro#10274) [ci] format update login flow to support Brave (withastro#10258) [ci] format improve link command (withastro#10257) Updates deprecated Node.js 16 github actions (withastro#10270) Fix Vitest check fail again (withastro#10266) [ci] format Adds auto completion of `astro:` events when adding or removing event listeners on `document` (withastro#10263) Update Vite to latest (withastro#10259) [ci] release (withastro#10236) [ci] format fix(i18n): localised index pages are overwritten (withastro#10250) fix: change strategy for route caching (withastro#10248) Fix TypeScript type definitions for `Code` component (withastro#10251) chang changeset (withastro#10253) Removes morph animations when setting transition:animate=none (withastro#10247) ...
Changes
astro link
command to handle new project creationastro link
command to handle existing project linking by name (vs. currently, only supporting an id passed as argument)Testing
Docs