Closed ryanccn closed 2 years ago
@ci010 @yuxuanchiadm
Hi, I think using the cross-spawn is a good idea to address those issues, but I want to keep the package dependencies clean.
What about adding an optional field with description to LaunchOption interface:
/**
* The spawn process function. Used for spawn the java process at the end. By default, it will be the spawn function from "child_process" module. You can use this option to change the 3rd party spawn like [cross-spawn](https://www.npmjs.com/package/cross-spawn)
*/
spawn?: (command: string, args?: ReadonlyArray<string>, options?: SpawnOptionsWithoutStdio): ChildProcessWithoutNullStreams;
And apply it to the launch function.
In this way, the user can choose what spawn to use, and it won't have breaking change to the package.
I did not checked the cross-spawn function type, but I suppose the argument types are the same. If the return type is different, you can just make the LaunchOption with generic:
interface LaunchOption<T = ChildProcessWithoutNullStreams>
{
// content before
/**
* The spawn process function. Used for spawn the java process at the end. By default, it will be the spawn function from "child_process" module. You can use this option to change the 3rd party spawn like [cross-spawn](https://www.npmjs.com/package/cross-spawn)
*/
spawn?: (command: string, args?: ReadonlyArray<string>, options?: SpawnOptionsWithoutStdio): T;
}
and the launch function is generic too:
export async function launch<T = ChildProcessWithoutNullStreams>(options: LaunchOption<T>): Promise<T>
But the problem here is that the native spawn simply won't work on Windows
If the package is cross platform it will have to use cross-spawn
(Or maybe vendorize dependencies?)
But the problem here is that the native spawn simply won't work on Windows
My launcher is currently using the native one, and I don't get trouble with it yet... Can you give some sample for when it won't work?
https://github.com/ryanccn/yamcl
On Windows the spawn fails but when I patched @xmcl/core
with the spawn
sourced from cross-spawn
it works
diff --git a/node_modules/@xmcl/core/dist/index.esm.js b/node_modules/@xmcl/core/dist/index.esm.js
index b2b68b1..2d87994 100644
--- a/node_modules/@xmcl/core/dist/index.esm.js
+++ b/node_modules/@xmcl/core/dist/index.esm.js
@@ -1,5 +1,5 @@
import { open, walkEntriesGenerator, openEntryReadStream } from '@xmcl/unzip';
-import { spawn } from 'child_process';
+import { spawn } from 'cross-spawn';
import { EventEmitter } from 'events';
import { access as access$1, link as link$1, readFile as readFile$1, writeFile as writeFile$1, mkdir as mkdir$1, constants, createReadStream, createWriteStream, existsSync } from 'fs';
import * as os from 'os';
diff --git a/node_modules/@xmcl/core/dist/index.js b/node_modules/@xmcl/core/dist/index.js
index 0cfb459..fe0b74a 100644
--- a/node_modules/@xmcl/core/dist/index.js
+++ b/node_modules/@xmcl/core/dist/index.js
@@ -935,7 +935,7 @@ async function launchServer(options) {
cwd = path.dirname(options.serverExectuableJarPath);
}
const spawnOption = { cwd, env: process.env, ...(options.extraExecOption || {}) };
- return child_process.spawn(args[0], args.slice(1), spawnOption);
+ return require('cross-spawn').spawn(args[0], args.slice(1), spawnOption);
}
/**
* Create a process watcher for a minecraft process.
https://github.com/ryanccn/yamcl
On Windows the spawn fails but when I patched
@xmcl/core
with thespawn
sourced fromcross-spawn
it works
Do you have any stdout/stderr or error stacktrace for that? If you use qq, you can chat with me in group 858391850
It returns an ENOENT
while the filename displayed is perfectly valid (the java.exe
binary)
Do you have any stdout/stderr or error stacktrace for that? If you use qq, you can chat with me in group 858391850
I don't use QQ 😂
Maybe we can just try the adding option to LaunchOption solution? It will perfectly work for you (even now you can actuall generateArguments and spawn process by yourself). I'll check out the cross-spawn later when I have time.
How about merging 😄
The change will be included in #226 (5.3.0)
Great 🎉
According to the README of
cross-spawn
:Node has issues when using spawn on Windows:
All these issues are handled correctly by cross-spawn. There are some known modules, such as win-spawn, that try to solve this but they are either broken or provide faulty escaping of shell arguments.
Notably, the launcher that I'm trying to create fails spectacularly on Windows.
(The tests pass.)