Skip to content
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

fix: don't exit the program if dependencies don't install #7052

Merged
merged 3 commits into from
May 12, 2023

Conversation

ematipico
Copy link
Member

@ematipico ematipico commented May 10, 2023

Changes

Closes #6600

This change affects only the "dependencies" phase, but in my opinion, we should do this for each step.

Testing

I manually changed the code to throw an error

Screenshot 2023-05-10 at 12 19 38

Docs

N/A

cc @withastro/maintainers-docs for feedback! Quick review of the new message

@changeset-bot
Copy link

changeset-bot bot commented May 10, 2023

🦋 Changeset detected

Latest commit: 561cace

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added the pkg: create-astro Related to the `create-astro` package (scope) label May 10, 2023
@Princesseuh
Copy link
Member

Princesseuh commented May 10, 2023

Hmm, I'm not sure I agree with this change. I think the warning is too subtle for users (we have to think that it's new users using create astro) and will result in many people asking for help on Discord due to thinking the deps installed when they didn't.

In my opinion, increasing the timeout so it happens less often (if that is in fact the issue) would be a better first step

@ematipico
Copy link
Member Author

In my opinion, increasing the timeout so it happens less often (if that is in fact the issue) would be a better first step

It could be a timeout issue, but it could also be another issue that we can't control. Even if we change the timeout, we are back to square one.

We can change the messaging, surely. I think we should not exit the program.

@Princesseuh
Copy link
Member

In my opinion, increasing the timeout so it happens less often (if that is in fact the issue) would be a better first step

It could be a timeout issue, but it could also be another issue that we can't control. Even if we change the timeout, we are back to square one.

We can change the messaging, surely. I think we should not exit the program.

I think I'd at least go with an error instead of a warning maybe?

@ematipico ematipico force-pushed the fix/installing-deps branch from b702cb5 to e16791e Compare May 10, 2023 12:28
@ematipico
Copy link
Member Author

In my opinion, increasing the timeout so it happens less often (if that is in fact the issue) would be a better first step

It could be a timeout issue, but it could also be another issue that we can't control. Even if we change the timeout, we are back to square one.
We can change the messaging, surely. I think we should not exit the program.

I think I'd at least go with an error instead of a warning maybe?

Done!

@ematipico ematipico requested review from delucis and Princesseuh May 10, 2023 14:29
Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Do you have a screenshot of the output after the changes?

@ematipico ematipico changed the title fix: don't exit the program is dependencies don't install fix: don't exit the program if dependencies don't install May 12, 2023
@ematipico
Copy link
Member Author

@delucis here's the updated (simulated) error

Screenshot 2023-05-12 at 09 45 47

@ematipico
Copy link
Member Author

Unfortunately, with the current APIs of cli-kit, we can't avoid that "Dependencies installed" message, and I didn't want to change cli-kit for the time being :(

What do you guys think?

@delucis
Copy link
Member

delucis commented May 12, 2023

Unfortunately, with the current APIs of cli-kit, we can't avoid that "Dependencies installed" message, and I didn't want to change cli-kit for the time being :(

Oh right… that’s unfortunate. I guess this PR is still much better than the whole thing crashing, but it would definitely be nice to avoid the confusing “Dependencies failed to install… Dependencies installed” message. How tricky is it to update this in a followup PR to cli-kit?

@ematipico
Copy link
Member Author

ematipico commented May 12, 2023

@delucis We can definitely do a follow up PR with the change!

@ematipico ematipico merged commit 8c14bff into main May 12, 2023
@ematipico ematipico deleted the fix/installing-deps branch May 12, 2023 15:00
@astrobot-houston astrobot-houston mentioned this pull request May 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: create-astro Related to the `create-astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dependencies installing with npm... error: Request timed out after one minute
5 participants