Skip to content

refactor: separate API facade from public contracts - #1787

Merged
Eduardo Villalpando Mello (edvilme) merged 9 commits into
mainfrom
api-refactor-types
Sep 16, 2026
Merged

Eduardo Villalpando Mello (edvilme) merged 9 commits into
mainfrom
api-refactor-types

Conversation

@edvilme

@edvilme Eduardo Villalpando Mello (edvilme) commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

This pull request clarifies the distinction between the Python Environments runtime API and the public TypeScript contracts used by the extension and npm package. The API is the runtime mechanism that extensions and automation use to execute operations, while the types define the public contracts and shared data shapes used throughout the codebase and exposed through @vscode/python-environments.

New file structure

  • src/types.ts

    • Canonical public contracts and interfaces, including PythonEnvironmentApi, environment and package shapes, manager contracts, and event/option types.
    • This file is intentionally contract-focused and does not contain extension lookup helpers or concrete implementations.
  • src/api.ts

    • Public runtime facade for consumers.
    • Re-exports the public contracts and public errors, keeps EXTENSION_ID, and provides PythonEnvironments.api() for resolving the VS Code extension API at runtime.
  • src/publicErrors.ts

    • Public runtime error definitions and guards that package consumers may need to handle explicitly.
  • src/extensionApi.ts

    • The extension-side implementation of PythonEnvironmentApi that is returned from activation.
    • This keeps the runtime implementation separate from the public facade and type declarations.
  • src/managers/common/models.ts

    • Concrete PythonEnvironmentImpl and PythonPackageImpl model implementations.
  • src/managers/common/registeredManagers.ts

    • Internal manager wrappers, preserving existing names such as InternalEnvironmentManager and InternalPackageManager to minimize PR impact.
  • Owning feature modules now define their own internal contracts instead of using the old src/internal.api.ts grab bag:

    • src/features/envManagers.ts owns environment-manager registry contracts and events.
    • src/features/projectManager.ts owns project-manager contracts and implementation.
    • src/features/creators/projectCreators.ts owns project-creator contracts and implementation.

Package and consumer behavior

  • The npm package continues to expose the existing public surface; this PR does not intentionally remove or rename package exports.
  • The extension does not consume @vscode/python-environments internally. Shared contracts come from local source files.
  • The package build copies the source-of-truth files from src/ into the package staging area before compiling.
  • CommonJS and ES module package entry points are covered by package smoke tests.

Other cleanup

  • Removed the obsolete examples/ sample extension. External extension authors should use the npm package/public contracts instead of copying a local sample that duplicated API files.
  • Updated documentation, packaging scripts, and release-gate checks to account for the split public API files.
  • Added webpack resolution support for .js import specifiers used by the package-compatible TypeScript sources.

Compatibility

No breaking changes are intended. Public names are preserved, internal wrapper names are preserved where possible, and the npm package remains the consumer-facing entry point for external extensions.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: de4cb63f-4a85-432d-a722-697b33e9d49a

This comment was marked as resolved.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: de4cb63f-4a85-432d-a722-697b33e9d49a
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: de4cb63f-4a85-432d-a722-697b33e9d49a

This comment was marked as resolved.

…rsions

Co-authored-by: edvilme <5952839+edvilme@users.noreply.github.com>
@edvilme

Copy link
Copy Markdown
Contributor Author

Copilot resolve the merge conflicts in this pull request

# Conflicts:
#	src/test/features/inlineScript/setupEnvironment.unit.test.ts

Co-authored-by: edvilme <5952839+edvilme@users.noreply.github.com>

Copilot AI commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Copilot resolve the merge conflicts in this pull request

Merged the latest main into this branch in fce6466. The only conflict was in src/test/features/inlineScript/setupEnvironment.unit.test.ts — I kept this branch's renamed import paths (EnvironmentManagers from features/envManagers, InternalEnvironmentManager from managers/common/registeredManagers, since internal.api.ts was renamed on this branch) and incorporated main's new extensionVersionCheck import. Typecheck, lint, and the full unit suite (2318 passing) all pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: de4cb63f-4a85-432d-a722-697b33e9d49a
@edvilme Eduardo Villalpando Mello (edvilme) changed the title refactor: separate public API concerns refactor: separate API facade from public contracts Sep 16, 2026
@edvilme
Eduardo Villalpando Mello (edvilme) marked this pull request as ready for review September 16, 2026 00:02
@bschnurr

Bill Schnurr (bschnurr) commented Sep 16, 2026

Copy link
Copy Markdown
Member

🔒 Automated review in progress — Bill Schnurr (@bschnurr) is auto-reviewing this PR.

@bschnurr

Copy link
Copy Markdown
Member

Result: ⚠️ partially-verified

Verification details

Verification: Isolated verification observed failures whose relationship to this PR could not be determined: Offline dependency bootstrap; this review is not fully verified. Isolated verification observed failures that were not classified as caused by this PR: Dependency and test discovery, Relative import and stale refactor import scan.

Summary: The source-copy and package entry-point assertions passed. The API package smoke tests and targeted extension unit tests could not run because offline dependency installation lacked `brace-expansion-2.1.4`, leaving `tsc` unavailable. An additional import scan failed only because the ad-hoc regex matched a commented-out import. Verification is therefore partial, with no PR-related failure observed.

Test runs: 1 passed, 3 failed, 2 not run

  • ⚠️ Not run | API package CommonJS and ES module smoke tests | npm run copy:sources && npm run compile && npm run test:package
  • ⚠️ Not run | Extension API and registered-manager version lookup unit tests | npm run compile-tests && node ./node_modules/mocha/bin/mocha.js --require source-map-support/register --require out/test/unittests.js --ui tdd --timeout 180000 out/test/extensionApi.unit.test.js out/test/managers/common/registeredManagers.versionLookup.unit.test.js
  • Failed | unrelated to this PR | Dependency and test discovery | printf 'PROFILE=%s\n' "$AUTOMATION_SANDBOX_PROFILE"; git diff --name-status HEAD^ HEAD; printf '\nROOT SCRIPTS\n'; node -e "const p=require('./package.json'); console.log(JSON.stringify(p.scripts,null,2))"; printf '\nAPI SCRIPTS\n'; node -e "const p=require('./api/package.json'); console.log(JSON.stringify(p.scripts,null,2))"; printf '\nDEPS\n'; test -d node_modules && echo root_node_modules=yes || echo root_node_modules=no; test -d api/node_modules && echo api_node_modules=yes || echo api_node_modules=no; printf '\nTEST CONFIG\n'; find . -maxdepth 3 ( -name 'mocha*.json' -o -name '.mocharc*' -o -name 'jest.config.*' ) -print | sort; printf '\nCHANGED TESTS\n'; git diff --name-only HEAD^ HEAD | grep -E '(^|/)(test|tests)/|.(unit|test|spec).' || true
  • Failed | relationship unknown | Offline dependency bootstrap | npm ci --offline && npm ci --offline --prefix api
  • Failed | unrelated to this PR | Relative import and stale refactor import scan | node - <<'NODE'
    const fs=require('fs'), path=require('path'), assert=require('assert');
    const files=[]; (function walk(d){for(const e of fs.readdirSync(d,{withFileTypes:true})){const p=path.join(d,e.name);e.isDirectory()?walk(p):p.endsWith('.ts')&&files.push(p)}})('src');
    const stale=[]; const unresolved=[];
    for(const file of files){const text=fs.readFileSync(file,'utf8'); if(/(?:from\s+|import\s*()['"][^&#x27;&quot;](?:internal.api|features/pythonApi)['"]/.test(text)) stale.push(file); for(const m of text.matchAll(/(?:from\s+|import\s()\s*'"['"]/g)){let s=m[1], base=path.resolve(path.dirname(file),s), candidates=[base,base+'.ts',path.join(base,'index.ts')]; if(s.endsWith('.js')) candidates=[base.slice(0,-3)+'.ts',base,path.join(base.slice(0,-3),'index.ts')]; if(!candidates.some(fs.existsSync)) unresolved.push(${file}: ${s});}}
    assert.deepStrictEqual(stale,[],stale imports: ${stale.join(&#x27;, &#x27;)}); assert.deepStrictEqual(unresolved,[],unresolved imports:\n${unresolved.join(&#x27;\n&#x27;)}); console.log(validated ${files.length} TypeScript files: no stale refactor imports or unresolved relative modules);
    NODE
  • Passed | Generated source and package entry-point assertions | node - <<'NODE'
    const fs=require('fs'), assert=require('assert');
    for (const [source,target] of [['src/api.ts','api/src/main.ts'],['src/types.ts','api/src/types.ts'],['src/publicErrors.ts','api/src/publicErrors.ts']]) assert.strictEqual(fs.readFileSync(target,'utf8'),fs.readFileSync(source,'utf8'),${target} differs from ${source});
    const api=fs.readFileSync('src/api.ts','utf8');
    assert.match(api,/export * from './types.js';/);
    assert.match(api,/export * from './publicErrors.js';/);
    const pkg=require('./api/package.json');
    assert.equal(pkg.main,'./out/cjs/main.cjs');
    assert.equal(pkg.exports.import.default,'./out/esm/main.mjs');
    assert.equal(pkg.exports.require.default,'./out/cjs/main.cjs');
    console.log('source copy and package entry-point assertions passed');
    NODE
⚠️ API package CommonJS and ES module smoke tests diagnostic output
copy:sources completed successfully, but compile:esm failed:
> tsc -b ./tsconfig.esm.json
sh: 1: tsc: not found
⚠️ Extension API and registered-manager version lookup unit tests diagnostic output
> vscode-python-envs@1.37.0 compile-tests
> tsc -p . --outDir out

sh: 1: tsc: not found
Dependency and test discovery diagnostic output
PROFILE=typescript
error: Could not access 'HEAD^'
root_node_modules=no
api_node_modules=no
Offline dependency bootstrap diagnostic output
npm error code ENOTCACHED
npm error request to https://packagefeedproxy.microsoft.io/npm/brace-expansion/-/brace-expansion-2.1.4.tgz failed: cache mode is 'only-if-cached' but no cached response is available.
Relative import and stale refactor import scan diagnostic output
AssertionError: unresolved imports:
src/test/mocks/vsc/index.ts: ./selection

The matched text is a commented-out export on line 8, so this is a false positive in the ad-hoc scanner.

@bschnurr Bill Schnurr (bschnurr) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approved via Review Center.

@bschnurr Bill Schnurr (bschnurr) added the review-auto:approved Automated review: no blocking findings (approval posted). label Sep 16, 2026
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: de4cb63f-4a85-432d-a722-697b33e9d49a
Comment thread src/features/projectManager.ts
Comment thread CONTRIBUTING.md
@bschnurr

Copy link
Copy Markdown
Member

Result: 🔴 could-not-verify

Verification details

Verification: Isolated verification observed failures that were not classified as caused by this PR: Extension TypeScript test compilation, API package compilation, Offline dependency bootstrap, Relative TypeScript import resolution probe. The relevant tests could not be fully run in the isolated environment; this review is not fully verified.

Summary: Verification was blocked by unavailable offline dependencies: `npm ci --offline` failed because `brace-expansion-2.1.4.tgz` was not cached. Consequently, extension and API compilation failed because `tsc` was unavailable, so targeted unit and package smoke tests could not run. An ad-hoc check confirmed all three public source files are copied byte-for-byte into `api/src`. Overall, the PR could not be meaningfully verified.

Test runs: 2 passed, 4 failed, 2 not run

  • Failed | unrelated to this PR | Extension TypeScript test compilation | npm run compile-tests
  • Failed | unrelated to this PR | API package compilation | npm run compile
  • ⚠️ Not run | Targeted API facade and manager unit tests | node ./node_modules/mocha/bin/mocha.js --config=./build/.mocha.unittests.json out/test/extensionApi.unit.test.js out/test/managers/common/registeredManagers.versionLookup.unit.test.js
  • ⚠️ Not run | API package CommonJS/ESM smoke test | npm run test:package
  • Failed | unrelated to this PR | Offline dependency bootstrap | npm ci --offline && npm ci --offline
  • Failed | unrelated to this PR | Relative TypeScript import resolution probe | node - <<'NODE'
    const fs = require('node:fs');
    const path = require('node:path');
    const files = [];
    (function walk(dir) { for (const e of fs.readdirSync(dir, {withFileTypes:true})) { const p=path.join(dir,e.name); if(e.isDirectory()) walk(p); else if(p.endsWith('.ts')) files.push(p); } })('src');
    const missing=[];
    for(const file of files){
    const text=fs.readFileSync(file,'utf8');
    for(const m of text.matchAll(/(?:from\s+|import\s*)'"['"]/g)){
    const spec=m[1]; if(!spec.startsWith('.')) continue;
    const base=path.resolve(path.dirname(file), spec);
    const candidates=spec.endsWith('.js') ? [base, base.slice(0,-3)+'.ts'] : [base, base+'.ts', path.join(base,'index.ts')];
    if(!candidates.some(p=>fs.existsSync(p))) missing.push(${file}: ${spec});
    }
    }
    if(missing.length){ console.error(missing.join('\n')); process.exit(1); }
    console.log(resolved relative imports in ${files.length} TypeScript source/test files);
    NODE
  • Passed | Dependency and test discovery | printf 'sandbox_profile=%s\n' "${AUTOMATION_SANDBOX_PROFILE:-unset}"; node --version; npm --version; printf 'root_node_modules='; test -d node_modules && echo present || echo missing; printf 'api_node_modules='; test -d api/node_modules && echo present || echo missing; printf '%s\n' '--- relevant scripts ---'; node -e "const p=require('./package.json'); console.log(JSON.stringify(p.scripts,null,2)); const a=require('./api/package.json'); console.log(JSON.stringify(a.scripts,null,2))"; printf '%s\n' '--- changed tests versus protected base ---'; base=$(git merge-base HEAD upstream/main 2>/dev/null || git merge-base HEAD origin/main 2>/dev/null || true); if test -n "$base"; then git diff --name-status "$base"...HEAD -- 'src/test/' 'api/test/' 'api/scripts/test-package.cjs'; else echo 'protected base ref unavailable'; fi
  • Passed | Public package source-copy verification | node ./scripts/copy-sources.cjs && cmp -s src/main.ts ../src/api.ts && cmp -s src/types.ts ../src/types.ts && cmp -s src/publicErrors.ts ../src/publicErrors.ts && echo 'copied all three public sources byte-for-byte'
Extension TypeScript test compilation diagnostic output
> vscode-python-envs@1.37.0 compile-tests
> tsc -p . --outDir out

sh: 1: tsc: not found
API package compilation diagnostic output
> @vscode/python-environments@1.4.0 compile:esm
> tsc -b ./tsconfig.esm.json && mve out/esm/main.js out/esm/main.mjs && node -e "require('fs').writeFileSync('out/esm/package.json', '{\"type\":\"module\"}')"

sh: 1: tsc: not found
⚠️ Targeted API facade and manager unit tests diagnostic output
Test compilation could not complete because the offline dependency bootstrap did not provide TypeScript.
⚠️ API package CommonJS/ESM smoke test diagnostic output
The package could not be compiled because API dependencies were unavailable.
Offline dependency bootstrap diagnostic output
npm error code ENOTCACHED
npm error request to https://packagefeedproxy.microsoft.io/npm/brace-expansion/-/brace-expansion-2.1.4.tgz failed: cache mode is 'only-if-cached' but no cached response is available.
Relative TypeScript import resolution probe diagnostic output
src/test/mocks/vsc/index.ts: ./selection

The probe matched a commented-out export rather than an executable import.

@bschnurr Bill Schnurr (bschnurr) added review-auto:changes-requested Automated review: posted blocking findings to address. and removed review-auto:approved Automated review: no blocking findings (approval posted). labels Sep 16, 2026
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: de4cb63f-4a85-432d-a722-697b33e9d49a
@bschnurr

Copy link
Copy Markdown
Member

Result: ⚠️ partially-verified

Verification details

Verification: The relevant tests could not be fully run in the isolated environment; this review is not fully verified.

Summary: Extension TypeScript compilation, webpack bundling, and 45 targeted unit tests passed. The API package smoke/type tests could not run because the package-local `mve` dependency was unavailable; compilation stopped with `sh: 1: mve: not found`. Verification is therefore partial.

Test runs: 4 passed, 1 not run

  • ⚠️ Not run | API package consumer and runtime smoke tests | ln -s ../node_modules node_modules && npm run copy:sources && npm run compile && npm_config_offline=true npm run test:package
  • Passed | Targeted API facade and moved implementation unit tests | node ./node_modules/mocha/bin/mocha.js --require source-map-support/register --require out/test/unittests.js --ui tdd --timeout 180000 out/test/extensionApi.unit.test.js out/test/features/envManagers.packageEvents.unit.test.js out/test/features/projectManager.initialize.unit.test.js out/test/features/projectManager.fileEvents.unit.test.js out/test/managers/common/registeredManagers.versionLookup.unit.test.js out/test/features/creators/autoFindProjects.unit.test.js
  • Passed | Dependency and test discovery | printf 'profile=%s\n' "$AUTOMATION_SANDBOX_PROFILE"; node --version; npm --version; printf 'root_node_modules='; test -d node_modules && echo present || echo missing; printf 'api_node_modules='; test -d api/node_modules && echo present || echo missing; printf '%s\n' '--- changed test files ---'; git diff --name-status "$(git merge-base HEAD upstream/main)" HEAD -- 'src/test/' 'api/test/' 'api/scripts/test-package.cjs'; printf '%s\n' '--- mocha config ---'; cat build/.mocha.unittests.json
  • Passed | Offline dependency bootstrap | npm ci --offline && npm ci --offline
  • Passed | Extension TypeScript and webpack build | npm run compile-tests && npm run compile
⚠️ API package consumer and runtime smoke tests diagnostic output
> npm run copy:sources
> npm run compile
> tsc -b ./tsconfig.esm.json && mve out/esm/main.js out/esm/main.mjs
sh: 1: mve: not found

@bschnurr Bill Schnurr (bschnurr) added review-auto:approved Automated review: no blocking findings (approval posted). and removed review-auto:changes-requested Automated review: posted blocking findings to address. labels Sep 16, 2026

@rchiodo Rich Chiodo (rchiodo) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approved via Review Center.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

debt Code quality issues review-auto:approved Automated review: no blocking findings (approval posted).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants