Don't register SIGTERM listener on Windows
Windows does not support SIGTERM.
References:
- https://docs.deno.com/api/deno/~/Deno.addSignalListener
- https://docs.deno.com/api/deno/~/Deno.build.os
- https://nodejs.org/api/process.html#processplatform
- https://learn.microsoft.com/en-us/previous-versions/xdkz3x12(v=vs.140)
Motivation
I'd like to use Effection on Windows/Deno.
Approach
Currently, Effection is unusable on Deno on Windows because Deno.addSignalListener("SIGTERM", ...) throws, because SIGTERM is not supported by Windows.
To solve this problem, I guard signal registration with a platform check.
@proctord-flal Is this a problem on both Deno and Node, or just Deno?
@cowboyd
Not a problem on Windows/Node, only on Windows/Deno.
node --version
v23.11.0
deno --version
deno 2.2.11 (stable, release, x86_64-pc-windows-msvc)
v8 13.5.212.10-rusty
typescript 5.7.3
systeminfo
OS Name: Microsoft Windows 11 Pro
OS Version: 10.0.26100 N/A Build 26100
main.mjs
import { main } from 'effection';
await main(function* () {
console.log("hello world");
});
package.json
{
"dependencies": {
"effection": "^3.5.0"
}
}
node main.mjs
hello world
deno main.mjs
$ deno --allow-all main.mjs
TypeError: Windows only supports ctrl-c (SIGINT) and ctrl-break (SIGBREAK), but got SIGTERM
at bindSignal (ext:deno_os/40_signals.js:14:10)
at Object.addSignalListener (ext:deno_os/40_signals.js:54:19)
at Object.deno (file:///C:/Users/duncp/git-repos/test/node_modules/effection/esm/lib/main.js:76:30)
at deno.next (<anonymous>)
at withHost (file:///C:/Users/duncp/git-repos/test/node_modules/effection/esm/lib/main.js:133:26)
at withHost.next (<anonymous>)
at file:///C:/Users/duncp/git-repos/test/node_modules/effection/esm/lib/main.js:71:20
at Generator.next (<anonymous>)
at file:///C:/Users/duncp/git-repos/test/node_modules/effection/esm/lib/instructions.js:65:21
at Generator.next (<anonymous>)
@proctord-flal Thanks for the clarification!
Given that, do we need to remove the SIGTERM handler for node?
Also, we should probably run the tests on windows.
This has been fixed by #1031 Thanks @proctord-flal! Even though we didn't end up going with this PR since most of the work was getting our CI setup to run on Windows, it did put it on our radar.
🤦 I should have used this as my starting point. Sorry I missed it.