Skip to content

fix(pi-fff): isomorphic bun/node SDK lazy-load#669

Merged
dmtrKovalenko merged 2 commits into
mainfrom
triage-bot/issue-668
Jul 15, 2026
Merged

fix(pi-fff): isomorphic bun/node SDK lazy-load#669
dmtrKovalenko merged 2 commits into
mainfrom
triage-bot/issue-668

Conversation

@gustav-fff

@gustav-fff gustav-fff commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Closes #668

Root cause

packages/pi-fff/src/index.ts:21 had a top-level import { FileFinder } from "@ff-labs/fff-node". @ff-labs/fff-node pulls ffi-rs and the optional per-platform native binary packages (@ff-labs/fff-bin-*) into its module graph. oh-my-pi statically resolves every extension import to validate the extension; the optional native package for a foreign platform is not on disk, so validation fails even though the runtime install would be fine.

Reporter also flagged that pi runs under both bun and node — but pi-fff only ever loaded the node SDK, so bun hosts paid the ffi-rs cost instead of using @ff-labs/fff-bun which shares the same FileFinderApi.

Fix

  • Drop the static FileFinder value import; keep @ff-labs/fff-node for type-only imports (erased at compile time, invisible to the static validator).
  • New detectRuntime() checks globalThis.Bun and process.versions.bun.
  • New loadSdk() does a variable-name await import(pkg) for the runtime-appropriate SDK, with a fallback to the other one. Variable-name import defeats static resolvers.
  • Result cached in sdkPromise so a single load survives concurrent tool calls.
  • ensureFinder() now await loadSdk() before FileFinder.create(...); the existing in-flight-promise guard from pi-fff: parallel ffgrep calls can hang indefinitely due to unsynchronized ensureFinder singleton #403 still serializes creation.
  • package.json: add @ff-labs/fff-bun as an optional peer dependency so bun-hosted pi pulls it in without forcing node-hosted pi to install it.

Both SDKs implement the identical FileFinderApi (packages/shared/fff-api.ts:528), so the call sites don't change.

Steps to reproduce

git checkout main
cd packages/pi-fff
bun install
# The static import chain is visible to any resolver that walks it:
node -e "console.log(require.resolve('@ff-labs/fff-node'))"
# oh-my-pi's static validator follows @ff-labs/fff-node -> ffi-rs -> optional native binary,
# which is missing for platforms other than the host and aborts validation.

Expected: extension loads under oh-my-pi.
Actual: validator errors on the unresolved optional native package.

How verified

  • cd packages/pi-fff && bun run typecheck — clean.
  • bun test test/ — 18/18 pass (added mock for @ff-labs/fff-bun alongside @ff-labs/fff-node since bun-hosted tests now prefer the bun SDK).
  • bun run format — no changes.

Not verified end-to-end against a real oh-my-pi install — that would need the pi validator in the loop. @dmtrKovalenko please smoke-test under omp before releasing.

Automated triage via Gustav. Honk-Honk 🪿

@gustav-fff

Copy link
Copy Markdown
Collaborator Author

[triage-bot] CI note: Spell Check with Typos fails on crates/fff-core/src/bigram_query.rs:202 (simpliestsimplest). Pre-existing on main — same failure on the latest main run: https://github.com/dmtrKovalenko/fff/actions/runs/29052646755. Unrelated to this diff.

Honk-Honk 🪿

@dmtrKovalenko
dmtrKovalenko force-pushed the triage-bot/issue-668 branch from a265a46 to fbdf9f8 Compare July 10, 2026 00:10
@gustav-fff gustav-fff changed the title fix: lazy-load @ff-labs/fff-node in pi-fff (#668) fix(pi-fff): isomorphic bun/node SDK lazy-load Jul 10, 2026
@dmtrKovalenko

Copy link
Copy Markdown
Owner

@gustav-fff iterate

@gustav-fff

Copy link
Copy Markdown
Collaborator Author

[triage-bot] DIRECTED: need a concrete direction — "iterate" is ambiguous. Candidates I see:

  1. Drop the import type from @ff-labs/fff-node entirely (belt-and-suspenders vs. a static validator that doesn't grok import type). Trade-off: lose the shared type surface, would need to re-import from @ff-labs/fff-bun or packages/shared/fff-api.ts.
  2. Remove the SDK cross-fallback in loadSdk() — on bun, falling back to @ff-labs/fff-node re-introduces the ffi-rs cost the PR is trying to avoid. Fail loud instead.
  3. Test end-to-end under real oh-my-pi (need a repro command from you — I don't have omp).
  4. Something else entirely.

Which one, or all?

Honk-Honk 🪿

@gustav-fff

Copy link
Copy Markdown
Collaborator Author

[triage-bot] DIRECTED: iterated on option 2 (drop the cross-runtime fallback). Pushed 24b02ed.

loadSdk() now imports only the runtime-appropriate SDK — no try/catch fallback. Rationale: on a bun host, falling back to @ff-labs/fff-node re-introduces the ffi-rs load cost this PR is meant to avoid, and silently masks packaging bugs (wrong SDK installed → we want a loud error, not a slow success).

Diff: -20 / +5 in packages/pi-fff/src/index.ts. Tests 18/18 pass.

Left options 1 (drop import type entirely) and 3 (real oh-my-pi smoke test) untouched — 1 is speculative without a validator that mis-handles import type, 3 needs a repro command from you. Say the word if you want either.

Honk-Honk 🪿

@dmtrKovalenko

Copy link
Copy Markdown
Owner

@gustav-fff hello - fix the CI

and verify that it works with oh-my-pi on whatever machine you are

@gustav-fff

Copy link
Copy Markdown
Collaborator Author

[triage-bot] DIRECTED:

CI fix: pushed 23d76afsimpliest → simplest in `crates/fff-core/src/bigram_query.rs:202`. Same fix as main (branch predates it); `typos .` clean locally now. The other CI slowness on this PR is unrelated to the diff — pi-fff only touches `packages/pi-fff/` + `packages/shared/` and doesn't exercise the Rust/Neovim/Python matrices.

oh-my-pi verify: no omp on this machine — closest available runtime is `pi` (earendil-works) which is node-shebanged here (`#!/usr/bin/env node`, so `detectRuntime()` returns `node`).

What I verified:

  • Extension registers under bun (bun -e harness): tools=["ffgrep","fffind"], commands=["fff-mode","fff-health","fff-rescan"], session_start fires cleanly. loadSdk() resolves @ff-labs/fff-bun — no ffi-rs pulled in.
  • import type from @ff-labs/fff-node erases: tsc --noEmit clean; the compiled JS has zero references to @ff-labs/fff-node, so any static extension validator walking the JS graph won't hit ffi-rs.
  • packages/pi-fff typecheck + bun test test/ (18/18) still green.
  • Cross-runtime dynamic import defeats static resolution: verified loadSdk() returns the correct SDK per detectRuntime(), no static string mentions of the other SDK path.

What I could NOT verify: end-to-end under the real oh-my-pi validator — omp isn't installable here without a repro command. If you can share the omp invocation that reproduced the original failure on `main`, I can rerun it against this branch.

Honk-Honk 🪿

@dmtrKovalenko

Copy link
Copy Markdown
Owner

okay @gustav-fff now rebase

Detect bun vs node at runtime and dynamically import the matching SDK
(@ff-labs/fff-bun on bun, @ff-labs/fff-node on node) via a variable
package name so oh-my-pi's static extension validator does not chase
ffi-rs' optional native binaries through the module graph.

Closes #668
Drop the cross-runtime SDK fallback in loadSdk. Falling back to the
node SDK on a bun host re-introduces the ffi-rs cost this PR is meant
to avoid, and hides packaging bugs where the correct SDK is missing.
@dmtrKovalenko
dmtrKovalenko force-pushed the triage-bot/issue-668 branch from 23d76af to be23eb5 Compare July 15, 2026 17:02
@gustav-fff

Copy link
Copy Markdown
Collaborator Author

[triage-bot] DIRECTED: rebased on origin/main. Head is now be23eb5 (was 23d76af).

  • Dropped 23d76af fix(typo): simpliest -> simplest — already upstream (patch contents identical, cleaned by git rebase).
  • Kept the two pi-fff commits: 14a756f (isomorphic runtime + lazy load) and be23eb5 (fail loud on wrong-runtime SDK).
  • Pushed with --force-with-lease.

Honk-Honk 🪿

@dmtrKovalenko
dmtrKovalenko merged commit d1dac82 into main Jul 15, 2026
52 checks passed
abhijit-s pushed a commit to abhijit-s/fff that referenced this pull request Jul 17, 2026
Brings in upstream 0.10.0 batch: SDK file-watcher exposure (dmtrKovalenko#674),
stable FffResult C accessors (dmtrKovalenko#681), pi-fff non-cwd + isomorphic
SDK fixes (dmtrKovalenko#622, dmtrKovalenko#669), picker_ui fuzzy-match highlight (dmtrKovalenko#673),
README FAQ (dmtrKovalenko#680), vimdoc regen (dmtrKovalenko#683).

Conflict resolution:
- All crate Cargo.toml + workspace: take ours (0.17.0). Upstream's
  0.10.0 is its own independent release line; the fork stays on 0.17.0.
  fff-mcp keeps its daemon-only deps (fff-ipc/dirs/libc).
- fff-core/src/lib.rs: keep BOTH the fork's content_staleness_recheck
  test module and upstream's new pub watch module (non-overlapping).
- install-mcp.sh: keep ours — the fork installs from HEAD/source and
  points at abhijit-s, so upstream's v0.10.0 release-pinning + SHAs
  do not apply.
- Cargo.lock: ours (no reconciliation needed on build).

Verified: build-daemon green; fff-search suite incl. upstream's new
watch:: tests + the fork's 4 content_staleness_recheck regressions;
daemon crate tests — all 0 failures.
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.

[Suggestion]: statically link imports

2 participants