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

Remove Deno.Signals enum #11909

Merged
merged 14 commits into from
Sep 6, 2021
Merged

Remove Deno.Signals enum #11909

merged 14 commits into from
Sep 6, 2021

Conversation

ry
Copy link
Member

@ry ry commented Sep 3, 2021

Fixes #11900

@ry ry requested review from kt3k and piscisaureus September 3, 2021 20:58
@kitsonk
Copy link
Contributor

kitsonk commented Sep 3, 2021

Using a string literal union, while it would still allow some unsupported signals, would greatly improve usability in editors. For example:

export type Signal = "SIGHUP" | "SIGINT";

export function kill(signal: Signal): void;

@caspervonb
Copy link
Contributor

Wonder if these should be case insensitive ala dom events?

@ry
Copy link
Member Author

ry commented Sep 5, 2021

@caspervonb I think it would just be more work - given the Signal string literal union, I think it will be clear that upper case is expected.

@caspervonb
Copy link
Contributor

@caspervonb I think it would just be more work - given the Signal string literal union, I think it will be clear that upper case is expected.

Would only be s.to_uppercase() call in the match but don't care that much, was just a passing thought.

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM from me

@bartlomieju bartlomieju requested a review from kitsonk September 5, 2021 20:41
@bartlomieju bartlomieju added this to the 1.14.0 milestone Sep 5, 2021
Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

LGTM

meszarosdezso added a commit to meszarosdezso/manual that referenced this pull request Oct 1, 2021
`Deno.signal` enum was removed in denoland/deno#11909, but the examples still use it. This updates the examples to use the `Signal` union type that replaced the enum.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move away from using enums for Deno.Signal API
6 participants