fix: make npx @ably/cli <command> work without the redundant ably#419
fix: make npx @ably/cli <command> work without the redundant ably#419mattheworiordan wants to merge 1 commit into
npx @ably/cli <command> work without the redundant ably#419Conversation
…oken `npx @ably/cli init` (and any other command) failed with "could not determine executable to run", forcing users into the verbose `npx -p @ably/cli ably init`. People expect a single command with npx; the fact that we couldn't do that was a packaging smell. Root cause: the package declared two `bin` entries pointing at different targets (`ably` -> ./bin/run.js, `ably-interactive` -> ./bin/ably-interactive). npm's bin resolver (libnpmexec/get-bin-from-manifest) only auto-selects an executable when either every bin points at the same target, or a bin is named after the unscoped package name (`cli`). Two differently-targeted bins, neither named `cli`, hit the "could not determine executable to run" branch. Fix: expose a single `bin` (`ably`) — the same shape every other scoped CLI uses (`@angular/cli` -> ng, `@vue/cli` -> vue). `npx @ably/cli <command>` now resolves deterministically. Verified against npm's own resolver (old manifest throws the exact error; new manifest returns `ably`) and end-to-end via `npx <packed tarball> --version`. Backwards compatible: `npx -p @ably/cli ably <command>` is unaffected, and run.js now drops a redundant leading `ably` token so the historical `npx @ably/cli ably <command>` form keeps working too. There is no top-level `ably` command, so a leading `ably` is unambiguously the repeated bin name. Global installs (`npm install -g @ably/cli` then `ably ...`) are unchanged. The `bin/ably-interactive` auto-restart wrapper script is retained in the package (still used by `ably interactive` and its tests) but is no longer installed as a separate global executable. The one runtime consumer of that binary, ably/cli-terminal-server, installs `@ably/cli@latest`, so its image build needs a companion one-line Dockerfile symlink landed alongside this change. Tests: a hermetic unit test re-implements npm's resolver and asserts the package always resolves to a single `ably` executable; an integration test covers the redundant-`ably` backwards-compatible invocation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis PR fixes Changes
Review Notes
|
There was a problem hiding this comment.
Review Summary
Clean, well-targeted fix. The root cause analysis is correct (two bins pointing at different targets trips npm's resolver) and the solution is the right one. No bugs found.
What this PR does:
- Removes the ably-interactive bin entry from package.json, leaving a single ably bin
- Adds backwards-compat stripping of a redundant leading 'ably' token in bin/run.js so 'npx @ably/cli ably ' keeps working
- Adds a unit test pinning the npm resolution algorithm and a regression guard if a second bin creeps back
- Adds an integration test covering the backwards-compat stripping
Correctness checks:
bin/run.js stripping logic: process.argv.splice(2, 1) fires before execute(), so oclif never sees the redundant token. The process.argv.includes('interactive') check runs after the strip, so 'npx @ably/cli ably interactive' correctly sets ABLY_INTERACTIVE_MODE. No issues.
Edge cases covered:
- No args: process.argv[2] is undefined, comparison is false — safe
- Double 'ably': only the first is stripped, second becomes the command name, oclif returns 'not found' — covered by test 3
- 'ably --help' strips to top-level help — correct
The algorithm copy in the unit test: The test reimplements npm's bin resolution from libnpmexec/lib/get-bin-from-manifest.js and the comment explicitly flags it as an intentional copy kept in sync. This is the right tradeoff: you get a regression guard that fails loudly if a second bin returns, at the cost of the algorithm potentially drifting from npm. Worth the call — this algorithm has been stable across npm major versions.
One thing to coordinate before merge:
The PR description flags it already, but worth calling out explicitly: removing ably-interactive as a global bin BREAKS THE TERMINAL SERVER until the companion Dockerfile symlink lands. These two PRs need to be merged and deployed in the right order. If the terminal server picks up this package version before the Dockerfile change, ably-interactive won't be on PATH and interactive mode will stop working.
This is already documented in the PR — just making sure it's not lost.
Verdict: Approve. The code is correct, the tests cover the key cases, and the coordination dependency is documented. No code changes requested.
Whilst watching someone today onboard and try the one click install, it became evident
npx -p @ably/cli ably inithas unnecessary duplication ofably, and looking at Neon for comparison, they've kept it simpler withnpx neonctl initwhich works well.Note trying
npx @ably/cli initfails with "could not determine executable to run", so you fall back tonpx -p @ably/cli ably init. The repeatedablyis pointless, and one command is what people expect from npx. We couldn't give them that, which is a smell. This fixes it.The cause: the package shipped two bins pointing at different files (
ablyandably-interactive). npm's resolver only auto-picks an executable when there's a single bin, or they all point at the same target. Two different ones, so npx can't choose. The fix is a singleablybin, same shape as every other scoped CLI (@angular/cligives young,@vue/cligives youvue).npx @ably/cli <command>now resolves cleanly.Nothing breaks:
npx @ably/cli <command>now worksnpx -p @ably/cli ably <command>still worksnpx @ably/cli ably <command>still works (run.js drops the redundant leadingably)npm install -g @ably/clithenably <command>is unchangedOne thing to coordinate:
ably-interactiveis no longer a separate global binary, though the wrapper script still ships in the package. The only runtime consumer is our terminal server, which installs@ably/cli@latest, so it needs a one-line Dockerfile symlink to land alongside this. Companion PR links to this PR.Tests: a unit test pins the single-bin resolution and fails loudly if a second bin creeps back, and an integration test covers the redundant-
ablyform.Docs follow-up if we're happy with this: the onboarding that says
npx -p @ably/cli ably initcan becomenpx @ably/cli init.