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

create-astro: Even if installation of dependencies fails, still prints Dependencies installed #8990

Closed
1 task done
skirianov opened this issue Nov 2, 2023 · 6 comments
Closed
1 task done
Assignees
Labels
- P2: nice to have Not breaking anything but nice to have (priority) pkg: create-astro Related to the `create-astro` package (scope)

Comments

@skirianov
Copy link
Contributor

Astro Info

Astro                    v3.4.3
Node                     v18.17.1
System                   macOS (arm64)
Package Manager          unknown
Output                   static
Adapter                  none
Integrations             none

If this issue only occurs in one browser, which browser is a problem?

No response

Describe the Bug

While running create-astro CLI on slow/bad network you can end up in Timeout error, but current implementation of spinner doesn't handle failed cases and will always print success message, even if error was shown.

This results in this confusing behaviour:

Screenshot 2023-11-02 at 22 17 29

spinner call from dependencies.ts file:
Screenshot 2023-11-02 at 22 25 46

Maybe it would be nice to handle such edge cases like slow internet, cause I think for my bad internet { cwd, timeout: 90_000, stdio: 'ignore' } timeout of 90s was not enough.

Also, maybe you could use your isOnline function every time before doing any network request, just to confirm that the user still has the internet.

function isOnline(): Promise<boolean> {
     return dns.lookup('[http://github.com](https://t.co/dZiD0COfJD)').then(
         () => true,
         () => false
      );
}

What's the expected result?

If due to my internet speed or internet connection interruption I was not able to install dependencies it should result in error.

Could also print something like
"Ah, oh, your internet connection is gone."
"Do you want to retry installing dependencies?"

Basically handling this edge case properly.

Link to Minimal Reproducible Example

N/A - CLI issue

Participation

  • I am willing to submit a pull request for this issue.
@github-actions github-actions bot added the needs triage Issue needs to be triaged label Nov 2, 2023
@lilnasy
Copy link
Contributor

lilnasy commented Nov 2, 2023

Thanks for looking into it!
Do you want to open a PR?

@lilnasy lilnasy added pkg: create-astro Related to the `create-astro` package (scope) - P2: nice to have Not breaking anything but nice to have (priority) and removed needs triage Issue needs to be triaged labels Nov 2, 2023
@skirianov
Copy link
Contributor Author

Still digging through the code. It looks like spinner itself never meant to be handling errors like this and always returns end message which is always success 😅

export async function spinner({ start, end, while: update = () => sleep(100), }, { stdin = process.stdin, stdout = process.stdout } = {}) {
    const act = update();
    const tooslow = Object.create(null);
    const result = await Promise.race([sleep(500).then(() => tooslow), act]);
    if (result === tooslow) {
        const loading = await gradient(chalk.green(start), { stdin, stdout });
        await act;
        loading.stop();
    }
    stdout.write(`${" ".repeat(5)} ${chalk.green("✔")}  ${chalk.green(end)}\n`); // always returns only success message
}

So, gotta look into it.

Wrt to this suggestion:

Could also print something like
"Ah, oh, your internet connection is gone."
"Do you want to retry installing dependencies?"

Let me know if you want to proceed with it, of course with proper print outs.

@lilnasy
Copy link
Contributor

lilnasy commented Nov 2, 2023

Would it retry installation? I think it would be fine to defer it to the user to do it manually afterwards.

In general, more ambiguous messaging would be better. We don't really know if it is slow/severed internet connection, or if its the server, the package manager, the OS, and the code to determine that info might be a bit much.

@skirianov
Copy link
Contributor Author

Gotcha. Then it will be easier, I'm updating https://github.com/withastro/cli-kit spinner code to handle errors and once that is ready, will get to the create-astro. So, you can assign it to me :)

@skirianov
Copy link
Contributor Author

@lilnasy fixed with #8993 - this PR depends on withastro/cli-kit#25

@lilnasy
Copy link
Contributor

lilnasy commented Nov 14, 2023

Closing as fixed by #9048.

@lilnasy lilnasy closed this as completed Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P2: nice to have Not breaking anything but nice to have (priority) pkg: create-astro Related to the `create-astro` package (scope)
Projects
None yet
Development

No branches or pull requests

2 participants