Skip to content

Comments

feat(cli): call watcher close at process exit#2550

Merged
underfin merged 2 commits intomainfrom
watch-cli-close
Nov 4, 2024
Merged

feat(cli): call watcher close at process exit#2550
underfin merged 2 commits intomainfrom
watch-cli-close

Conversation

@underfin
Copy link
Contributor

@underfin underfin commented Nov 1, 2024

@netlify
Copy link

netlify bot commented Nov 1, 2024

Deploy Preview for rolldown-rs canceled.

Name Link
🔨 Latest commit cde5b18
🔍 Latest deploy log https://app.netlify.com/sites/rolldown-rs/deploys/6724aa4b016a650008a0de70

@IWANABETHATGUY
Copy link
Member

Can we add a test to ensure that if there are any errors, the watcher can be closed with a zero exit code?

@underfin underfin enabled auto-merge November 1, 2024 10:16
@underfin underfin disabled auto-merge November 1, 2024 10:18
@underfin
Copy link
Contributor Author

underfin commented Nov 1, 2024

Can we add a test to ensure that if there are any errors, the watcher can be closed with a zero exit code?

I tested it, because the watcher.close is a async,onExit maybe could't wait watcher.close finish, it is unstable. It is difficult to add a stable test for it, so let us skip it at now.

@underfin underfin enabled auto-merge November 1, 2024 10:43
@underfin underfin disabled auto-merge November 1, 2024 12:48
@underfin underfin marked this pull request as draft November 1, 2024 12:58
@underfin
Copy link
Contributor Author

underfin commented Nov 4, 2024

I get some info after research, it is difficult to ensure async task is finished at process exit. But here has a helper info us to out.

If you ^C a process, the kernel sends a SIGINT to the process group; that usually terminates both the parent and its children, grandchildren, and so on.

So we only need to care about is using watch cli bin at child process, Like test watch-cli using child process, it is unstable at process exit. So here i suggest to skip it at now.

@underfin underfin marked this pull request as ready for review November 4, 2024 08:40
@underfin underfin added this pull request to the merge queue Nov 4, 2024
@underfin
Copy link
Contributor Author

underfin commented Nov 4, 2024

tracking test at #2584

Merged via the queue into main with commit 33b2880 Nov 4, 2024
@underfin underfin deleted the watch-cli-close branch November 4, 2024 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants