Skip to content

Commit e88b3f7

Browse files
ARHAEEMclaude
andcommitted
fix(build): generalize the napi-binary vendoring fix to @ngrok/ngrok (F1)
Code review of the impit fix (3b71a39) found the identical unfixed defect one package over: @ngrok/ngrok declares 13 per-platform optionalDependencies for its own NAPI binding (@ngrok/ngrok-win32-x64-msvc, etc.), and prepare-package-deps.mjs was still copying only the bare @ngrok/ngrok folder. A packaged-VSIX user who enables the ngrok tunnel provider would hit "Cannot find native binding" the same way AIRTABLE_HTTP_CLIENT=impit did before that fix. Rather than hardcode a second package name, generalize the mechanism: after copying any package in packagesToCopy, read ITS OWN optionalDependencies (from the already-copied dist/node_modules/<name>/package.json, sidestepping strict `exports` maps like otpauth's) and vendor whichever resolve from this machine. Entries shaped like NAPI platform splits (child name prefixed with the parent's own name — impit-<platform>, @ngrok/ngrok-<platform>) trigger a loud aggregate warning if none resolve; other optional deps (e.g. patchright's fsevents, a genuinely-optional macOS file watcher) are still vendored when available but don't warrant the same alarm when missing. This closes the whole class of bug for every currently-vendored dependency and any future addition, not just the two packages known to hit it today. Verified against a real rebuilt VSIX (pnpm packx:no-bump): both dist/node_modules/impit-win32-x64-msvc/impit-node.win32-x64-msvc.node and dist/node_modules/@ngrok/ngrok-win32-x64-msvc/ngrok.win32-x64-msvc.node are now present; `import('@ngrok/ngrok')` from the extracted VSIX tree loads the real native binding (connect(), Session, etc. present). Falsification control: moving the vendored @ngrok/ngrok-win32-x64-msvc folder away reproduces `Cannot find module '@ngrok/ngrok-win32-x64-msvc'` (MODULE_NOT_FOUND) — the exact shape the repo's own isNativeMissingError() regex already matches — and restoring it fixes the load again. Confirmed the scope boundary the reviewer asked about: ngrok.js's isNgrokNativeAvailable()/isSetupComplete() already degrade this failure to a typed NgrokNativeMissingError / {ready:false, reason} instead of crashing anything — verified live by running the real ngrok.js from a directory tree with @ngrok/ngrok genuinely unresolvable (exit 0, no crash, typed error surfaced). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 3b71a39 commit e88b3f7

1 file changed

Lines changed: 55 additions & 25 deletions

File tree

scripts/prepare-package-deps.mjs

Lines changed: 55 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -159,39 +159,69 @@ function copyPackage(packageName) {
159159
return true;
160160
}
161161

162-
for (const packageName of packagesToCopy) {
163-
copyPackage(packageName);
164-
}
162+
/**
163+
* Some vendored packages (impit, @ngrok/ngrok, and potentially future
164+
* additions) ship their compiled NAPI (.node) binary in SEPARATE per-platform
165+
* packages declared as their OWN optionalDependencies (e.g.
166+
* impit-win32-x64-msvc, @ngrok/ngrok-darwin-arm64) — pnpm only installs the
167+
* variant matching the current build machine. Copying just the parent
168+
* package folder copies the pure-JS loader but never the native binary it
169+
* `require()`s at runtime, so the feature throws "Cannot find native
170+
* binding" in the packaged VSIX despite being selectable/enabled. This bit
171+
* us for impit first; rather than hardcode a second package name when it
172+
* turned out `@ngrok/ngrok` has the exact same shape, this generalizes: walk
173+
* whatever optionalDependencies the ALREADY-COPIED package itself declares
174+
* (read from dist/node_modules/<name>/package.json — sidesteps packages with
175+
* a strict `exports` map, like otpauth, that don't expose ./package.json via
176+
* require.resolve) and vendor whichever of those resolve from this machine.
177+
*
178+
* Not every optionalDependency is a platform-binary split, though (e.g.
179+
* patchright declares `fsevents`, a genuinely-optional macOS file watcher
180+
* its own code already handles being absent) — those are still vendored
181+
* when resolvable (harmless, and correct when it IS available), but a
182+
* missing one doesn't warrant the loud "will crash at runtime" warning.
183+
* NAPI-style platform splits conventionally name the child after the
184+
* parent (impit -> impit-<platform>, @ngrok/ngrok -> @ngrok/ngrok-<platform>);
185+
* only THOSE trigger the stronger aggregate warning when none resolve.
186+
*/
187+
function vendorOptionalDependencies(packageName) {
188+
const pkgJsonPath = join(extensionNodeModules, packageName, 'package.json');
189+
if (!existsSync(pkgJsonPath)) return;
165190

166-
// impit ships its compiled NAPI (.node) binary in a SEPARATE per-platform
167-
// package (e.g. impit-win32-x64-msvc), declared as an optionalDependency of
168-
// `impit` itself — pnpm only installs the variant matching the current
169-
// build machine. Copying just the `impit` package folder (above) copies the
170-
// pure-JS loader but never the native binary it `require()`s at runtime, so
171-
// AIRTABLE_HTTP_CLIENT=impit would throw "Cannot find native binding" in the
172-
// packaged VSIX despite the setting being selectable. Read the platform
173-
// package names straight from impit's own package.json (rather than
174-
// hardcoding them) so a future impit version adding/dropping targets can't
175-
// silently drift this list out of sync again.
176-
const impitTarget = join(extensionNodeModules, 'impit');
177-
if (existsSync(join(impitTarget, 'package.json'))) {
178-
const impitPkg = JSON.parse(readFileSync(join(impitTarget, 'package.json'), 'utf8'));
179-
const impitPlatformPackages = Object.keys(impitPkg.optionalDependencies || {});
180-
let copiedAny = false;
181-
for (const platformPkg of impitPlatformPackages) {
182-
if (copyPackage(platformPkg)) copiedAny = true;
191+
let pkg;
192+
try {
193+
pkg = JSON.parse(readFileSync(pkgJsonPath, 'utf8'));
194+
} catch {
195+
return; // malformed — nothing we can do, the parent package is still vendored
183196
}
184-
if (!copiedAny) {
197+
198+
const optionalDeps = Object.keys(pkg.optionalDependencies || {});
199+
if (optionalDeps.length === 0) return;
200+
201+
const platformShaped = optionalDeps.filter((dep) => dep.startsWith(`${packageName}-`));
202+
203+
for (const dep of optionalDeps) {
204+
copyPackage(dep);
205+
}
206+
207+
const anyPlatformCopied = platformShaped.some((dep) => existsSync(join(extensionNodeModules, dep)));
208+
if (platformShaped.length > 0 && !anyPlatformCopied) {
185209
console.warn(
186-
'⚠ impit was vendored but none of its platform-specific native binary packages ' +
187-
`(${impitPlatformPackages.join(', ')}) could be resolved — ` +
188-
'AIRTABLE_HTTP_CLIENT=impit will fail with "Cannot find native binding" in this VSIX. ' +
210+
`⚠ ${packageName} was vendored but none of its platform-specific native binary packages ` +
211+
`(${platformShaped.join(', ')}) could be resolved — any feature that depends on ` +
212+
`${packageName}'s native binding will fail with "Cannot find native binding" in this VSIX. ` +
189213
'Run `pnpm install` on this machine before packaging so the current platform\'s ' +
190-
'impit-<platform> optional dependency is present in node_modules.'
214+
`${packageName}-<platform> optional dependency is present in node_modules.`
191215
);
192216
}
193217
}
194218

219+
for (const packageName of packagesToCopy) {
220+
if (copyPackage(packageName)) {
221+
vendorOptionalDependencies(packageName);
222+
}
223+
}
224+
195225
// Copy @airtable-formula/language-services (workspace package — not on npm,
196226
// so it cannot be resolved from the mcp-server scope like patchright/otpauth).
197227
// The extension's bundled extension.js require()s it at runtime; without this

0 commit comments

Comments
 (0)