From 4907d1d695242ba25a3e6011814a4b6208f2a545 Mon Sep 17 00:00:00 2001 From: charliewwdev Date: Tue, 1 Sep 2026 17:08:28 +0800 Subject: [PATCH] fix(npm): make the Dart fallback usable on Windows and diagnose failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The native-binary spawn crash was fixed in #55, but the fallback it reaches was itself broken in three ways (issues #45, #49, #50). - `spawn('dart', ...)` cannot launch the Windows SDK entrypoints, which are .bat scripts CreateProcess refuses to execute. It failed with the same `spawn UNKNOWN` reported against the native path, and had no error handling, so the process crashed. Spawn through a shell on Windows, quoting arguments by hand since a shell does none, and guard both the synchronous throw and the async 'error' event. - The prerequisite check only looked for Dart. The vendored package depends on package:flutter, so `dart pub get` can never resolve it and the run died with "Couldn't resolve the package 'flutter_skill'" — which names the wrong problem. Require Flutter and say so. - `flutter pub get` failures were swallowed by an empty catch, so the real reason was discarded and the failure surfaced later as an unrelated import error. Report the output and stop. Also stops a failed binary download from poisoning later installs: a 404 or connection error left behind the empty file createWriteStream had already opened, and the "already installed" check only tested for existence, so the download was never retried. Remove the partial file on failure and treat a zero-byte file as absent. --- packaging/npm/bin/cli.js | 75 ++++++++++++++++++++++------ packaging/npm/scripts/postinstall.js | 12 ++++- 2 files changed, 69 insertions(+), 18 deletions(-) diff --git a/packaging/npm/bin/cli.js b/packaging/npm/bin/cli.js index d0c33f62..0bff37d2 100755 --- a/packaging/npm/bin/cli.js +++ b/packaging/npm/bin/cli.js @@ -151,35 +151,61 @@ function runNativeBinary(binaryPath) { process.on('SIGTERM', () => server.kill('SIGTERM')); } +const isWindows = process.platform === 'win32'; + +// The Flutter and Dart entrypoints on Windows are `.bat` scripts, which +// CreateProcess cannot execute directly — spawn() fails with UNKNOWN or ENOENT +// unless it goes through a shell. A shell does no argument quoting for us, so +// anything containing whitespace has to be quoted by hand. +function quoteArg(value) { + return isWindows && /[\s&|<>^()]/.test(value) ? `"${value}"` : value; +} + // Run using Dart function runWithDart() { const dartDir = path.join(__dirname, '..', 'dart'); const serverScript = path.join(dartDir, 'bin', 'server.dart'); - // Check if Dart is installed - try { - execSync('dart --version', { stdio: 'ignore' }); - } catch (e) { - console.error('Error: Dart SDK not found. Please install Flutter/Dart first.'); + if (!fs.existsSync(serverScript)) { + console.error('Error: Server script not found at:', serverScript); + console.error('The npm package looks incomplete — try reinstalling flutter-skill.'); + process.exit(1); + } + + // The vendored package depends on package:flutter, so `dart pub get` can + // never resolve it ("Because flutter_skill requires the Flutter SDK, version + // solving failed"). Checking only for Dart let that failure through and the + // run then died with a confusing "Couldn't resolve the package + // 'flutter_skill'" instead of naming the real prerequisite. + if (!checkFlutter()) { + console.error('Error: Flutter SDK not found.'); + console.error('flutter-skill runs on the Flutter SDK — the Dart SDK alone cannot'); + console.error('resolve its dependencies. Install Flutter and make sure it is on PATH:'); console.error(' https://docs.flutter.dev/get-started/install'); process.exit(1); } - // Check if server script exists - if (!fs.existsSync(serverScript)) { - console.error('Error: Server script not found at:', serverScript); + try { + execSync('dart --version', { stdio: 'ignore' }); + } catch (e) { + console.error('Error: `dart` was not found on PATH, but `flutter` was.'); + console.error('Add the Flutter SDK\'s bin directory to PATH so `dart` resolves too.'); process.exit(1); } - // Get dependencies silently + // A failure here used to be swallowed, leaving the run to fail later with an + // unrelated-looking import error. Report it and stop. try { - const pubCmd = checkFlutter() ? 'flutter' : 'dart'; - execSync(`${pubCmd} pub get`, { + execSync('flutter pub get', { cwd: dartDir, stdio: ['ignore', 'pipe', 'pipe'] }); } catch (e) { - // Ignore pub get errors + console.error('[flutter-skill] `flutter pub get` failed in', dartDir); + const details = ((e.stderr || '') + (e.stdout || '')).toString().trim(); + if (details) console.error(details); + console.error('[flutter-skill] Cannot start without resolved dependencies.'); + process.exit(1); } // Start with Dart @@ -188,10 +214,27 @@ function runWithDart() { args.push('server'); } - const dartArgs = ['run', serverScript, ...args]; - const server = spawn('dart', dartArgs, { - cwd: dartDir, - stdio: 'inherit' + const dartArgs = ['run', serverScript, ...args].map(quoteArg); + + // Same launch-failure handling as the native path: spawn() reports failures + // synchronously on some platforms and only via the 'error' event on others. + // There is no further fallback beyond Dart, so both paths exit with a + // diagnostic rather than crashing on an uncaught exception. + let server; + try { + server = spawn('dart', dartArgs, { + cwd: dartDir, + stdio: 'inherit', + shell: isWindows + }); + } catch (err) { + console.error(`[flutter-skill] Failed to start the Dart runtime (${err.code || err.message})`); + process.exit(1); + } + + server.on('error', (err) => { + console.error(`[flutter-skill] Failed to start the Dart runtime (${err.code || err.message})`); + process.exit(1); }); server.on('close', (code) => { diff --git a/packaging/npm/scripts/postinstall.js b/packaging/npm/scripts/postinstall.js index 74edb0af..ed08f5bd 100644 --- a/packaging/npm/scripts/postinstall.js +++ b/packaging/npm/scripts/postinstall.js @@ -39,6 +39,9 @@ function downloadBinary(url, destPath) { } if (response.statusCode !== 200) { + // createWriteStream has already created an empty file. Leaving it + // behind makes every later install believe the binary is present. + file.close(() => fs.unlink(destPath, () => {})); reject(new Error(`HTTP ${response.statusCode}`)); return; } @@ -68,7 +71,10 @@ function downloadBinary(url, destPath) { console.log('\n[flutter-skill] Native binary installed successfully!'); resolve(destPath); }); - }).on('error', reject); + }).on('error', (err) => { + file.close(() => fs.unlink(destPath, () => {})); + reject(err); + }); }; request(url); @@ -84,7 +90,9 @@ async function main() { const localPath = path.join(binDir, `${binaryName}-v${VERSION}`); - if (fs.existsSync(localPath)) { + // A zero-byte file is the residue of an interrupted or 404'd download, not an + // installed binary. Treat it as absent so the download is retried. + if (fs.existsSync(localPath) && fs.statSync(localPath).size > 0) { // Re-apply execute permission in case a previous install left the binary // without +x (e.g. chmod failed silently inside a restricted npm sandbox). try { fs.chmodSync(localPath, 0o755); } catch (_) {}