Skip to content

Comments

fix(nodejs): add CJS compatibility for VS Code extensions#546

Open
darthmolen wants to merge 1 commit intogithub:mainfrom
darthmolen:fix/528-cjs-compatibility
Open

fix(nodejs): add CJS compatibility for VS Code extensions#546
darthmolen wants to merge 1 commit intogithub:mainfrom
darthmolen:fix/528-cjs-compatibility

Conversation

@darthmolen
Copy link

Summary

Fixes #528getBundledCliPath() uses import.meta.resolve("@github/copilot/sdk") which throws in CJS contexts (VS Code extensions bundled with esbuild format:"cjs").

Changes:

  • Replace import.meta.resolve with createRequire + module resolution path walking (works in both ESM and CJS)
  • Add bundled CJS output (dist/cjs/index.cjs) via esbuild
  • Add conditional exports in package.json (import + require)
  • Add CJS compatibility test suite (4 tests)

Problem

The @github/copilot package only exposes an ESM "import" condition for its ./sdk subpath export — no "require" condition. This means:

  1. import.meta.resolve("@github/copilot/sdk") — works in ESM only
  2. require.resolve("@github/copilot/sdk") — fails (Package subpath './sdk' is not defined by "exports")
  3. require.resolve("@github/copilot/package.json") — also fails (not exported)

When a consumer bundles the SDK with esbuild format:"cjs", import.meta becomes an empty object, and import.meta.resolve is undefined.

Solution

Instead of resolving through the @github/copilot package's strict exports, we:

  1. Use createRequire(import.meta.url ?? pathToFileURL(__filename).href)import.meta.url in ESM, __filename fallback in CJS
  2. Walk require.resolve.paths("@github/copilot") to get the Node module search directories
  3. Check each path for @github/copilot/index.js using existsSync

This avoids the ESM-only exports entirely and works in both module systems.

Test plan

  • Existing unit tests pass (19/19 passing, 3 pre-existing CLI spawn failures unchanged)
  • New CJS tests pass (4/4):
    • CJS dist file exists
    • ESM dist file exists
    • CJS build is requireable and exports CopilotClient
    • CopilotClient constructor resolves bundled CLI path in CJS context
  • Manual: bundle SDK in a VS Code extension with esbuild format:"cjs", verify getBundledCliPath() works

The SDK's getBundledCliPath() used import.meta.resolve() which fails
when consumed via CJS bundlers (esbuild format:"cjs", webpack). This
breaks VS Code extensions that bundle dependencies with esbuild.

Changes:
- Replace import.meta.resolve with createRequire + resolution path
  walking (works in both ESM and CJS contexts)
- Add bundled CJS output (dist/cjs/index.cjs) via esbuild
- Add conditional exports in package.json (import + require)
- Add CJS compatibility test suite

The @github/copilot package only exposes ESM exports for ./sdk, so
require.resolve cannot resolve it directly. Instead, we walk the
module resolution paths and check for the package directory.

Fixes github#528
Copilot AI review requested due to automatic review settings February 23, 2026 18:45
@darthmolen darthmolen requested a review from a team as a code owner February 23, 2026 18:45
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a critical CJS compatibility issue (#528) where getBundledCliPath() failed in VS Code extensions bundled with esbuild format:"cjs". The root cause was using import.meta.resolve(), an ESM-only API that becomes undefined in CJS contexts.

Changes:

  • Replaced import.meta.resolve() with a createRequire() + path walking approach that works in both ESM and CJS environments
  • Added CJS build output (dist/cjs/index.cjs) via esbuild bundling
  • Updated package.json with conditional exports supporting both import and require conditions
  • Added 4 new CJS compatibility tests to verify the fix

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
nodejs/src/client.ts Refactored getBundledCliPath() to use createRequire() with __filename fallback instead of import.meta.resolve(), making it work in CJS bundles
nodejs/package.json Added conditional exports with separate import (ESM) and require (CJS) entry points, updated main field to point to ESM build
nodejs/esbuild-copilotsdk-nodejs.ts Added CJS build configuration that bundles to dist/cjs/index.cjs with external dependencies marked
nodejs/test/cjs-compat.test.ts New test suite with 4 tests validating CJS build exists, is requireable, exports CopilotClient, and resolves CLI path correctly

Comment on lines +45 to +50
// Verify that new CopilotClient({ cliUrl: "8080" }) doesn't throw
// (constructor calls getBundledCliPath() for the default cliPath)
const script = `
const sdk = require(${JSON.stringify(cjsEntryPoint)});
// Use cliUrl to avoid actually spawning the CLI,
// but the constructor still evaluates getBundledCliPath() for cliPath default
Copy link

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test comment states that using cliUrl: "8080" avoids spawning the CLI but "the constructor still evaluates getBundledCliPath() for cliPath default". However, looking at the constructor implementation, when cliUrl is provided, the cliPath is still resolved via getBundledCliPath() even though it won't be used (since isExternalServer will be true).

This test correctly validates that getBundledCliPath() works in a CJS context, but it may not catch scenarios where the @github/copilot package is not installed or available in node_modules, which is common in VS Code extension bundles that don't include node_modules. Consider adding a test case that simulates this scenario or clarifying in the comment that the test assumes @github/copilot is installed in node_modules.

Suggested change
// Verify that new CopilotClient({ cliUrl: "8080" }) doesn't throw
// (constructor calls getBundledCliPath() for the default cliPath)
const script = `
const sdk = require(${JSON.stringify(cjsEntryPoint)});
// Use cliUrl to avoid actually spawning the CLI,
// but the constructor still evaluates getBundledCliPath() for cliPath default
// Verify that new CopilotClient({ cliUrl: "8080" }) doesn't throw.
// In the current implementation, the constructor still resolves a default
// cliPath via getBundledCliPath(), even when cliUrl is provided, so this
// test exercises getBundledCliPath() in a pure CJS context.
//
// NOTE: This test assumes that the bundled @github/copilot CLI package is
// present in node_modules. It does NOT simulate environments (e.g. some
// VS Code extension bundles) where @github/copilot is absent.
const script = `
const sdk = require(${JSON.stringify(cjsEntryPoint)});
// Use cliUrl to avoid actually spawning the CLI process; we only care
// that constructing the client (and resolving the bundled CLI path)
// does not throw in a CJS-only environment.

Copilot uses AI. Check for mistakes.
Comment on lines +134 to 144
const searchPaths = require.resolve.paths("@github/copilot") ?? [];
for (const base of searchPaths) {
const candidate = join(base, "@github", "copilot", "index.js");
if (existsSync(candidate)) {
return candidate;
}
}
throw new Error(
"Could not find @github/copilot package. Ensure it is installed."
);
}
Copy link

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation assumes that require.resolve.paths("@github/copilot") will return a non-empty array. However, in some bundled scenarios or when using alternative package managers, this method might return null or an empty array, which would cause the function to throw immediately without providing helpful debugging information.

Consider adding logging or a more descriptive error message that explains what paths were searched. For example:

const searchPaths = require.resolve.paths("@github/copilot") ?? [];
if (searchPaths.length === 0) {
    throw new Error(
        "Could not determine module resolution paths for @github/copilot. " +
        "Ensure the package is installed or provide an explicit cliPath option."
    );
}
// ... existing loop ...

This would help developers diagnose issues more quickly.

Copilot uses AI. Check for mistakes.
Comment on lines +31 to +38
// Mark dependencies as external — they're resolved at runtime by the consumer
external: [
"vscode-jsonrpc",
"vscode-jsonrpc/*",
"zod",
"@github/copilot",
"@github/copilot/*",
],
Copy link

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CJS build configuration marks @github/copilot and @github/copilot/* as external dependencies, which means they won't be bundled. This is correct since getBundledCliPath() needs to resolve the actual installed package in node_modules at runtime.

However, this creates a potential issue: if a consumer (like a VS Code extension) bundles the SDK and doesn't include @github/copilot in their node_modules at runtime, getBundledCliPath() will fail. The PR description mentions this scenario but doesn't provide a clear solution beyond passing an explicit cliPath.

Consider documenting this requirement prominently in the README or adding a fallback behavior when the package can't be found.

Copilot uses AI. Check for mistakes.
@SteveSandersonMS
Copy link
Contributor

Thanks @darthmolen. I was expecting we could produce just a single build output. Supporting both cjs and esm will be an ongoing cost as people would encounter different issues across the two. Do you have a reason to think we need to support both and not just CJS?

@SteveSandersonMS
Copy link
Contributor

Perhaps the best outcome altogether would be having only a single ESM build, but putting in place whatever fallback logic is needed to get __filename-based path resolution to work if you are loading in a shimmed CJS environment.

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.

Node SDK: getBundledCliPath() breaks in CJS bundles (VS Code extensions)

2 participants