Add winapp node jsbindings for typed JS/TS WinRT bindings#536
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new “JS/TS WinRT bindings” pipeline to WinAppCli, exposing it both as (1) npm-wrapper-only CLI surfaces (winapp init --js-bindings*, winapp node jsbindings ...) and (2) persisted workspace configuration via an opt-in jsBindings: block in winapp.yaml, backed by dynwinrt/dynwinrt-codegen orchestration and a winmd discovery lockfile.
Changes:
- Introduces
winapp node jsbindings add|generatecommands andinit --js-bindings*flags (npm-only gated viaWINAPP_CLI_CALLER). - Adds new services/models for jsBindings config parsing/splicing, WinMD discovery/partitioning, lockfile IO, package manager detection, and package.json runtime-dependency injection.
- Updates docs, CLI schema, samples, and adds extensive unit/integration test coverage for the new pipeline.
Reviewed changes
Copilot reviewed 66 out of 68 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/winapp-npm/src/winapp-commands.ts | Exposes new init flags + node jsbindings add/generate through the npm programmatic API wrapper. |
| src/winapp-npm/src/cli.ts | Adds node jsbindings to the npm shim’s command routing/help and forwards to the native CLI. |
| src/winapp-npm/scripts/generate-docs.mjs | Improves JSDoc extraction to avoid truncation at @… and tweaks generated header. |
| src/winapp-npm/scripts/generate-commands.mjs | Escapes problematic doc text and adds naming overrides for generated TS surface. |
| src/winapp-npm/README.md | Documents node jsbindings add in the npm package readme. |
| src/winapp-npm/package.json | Adds @microsoft/dynwinrt-codegen runtime dependency to the npm wrapper. |
| src/winapp-npm/package-lock.json | Locks @microsoft/dynwinrt-codegen dependency and metadata. |
| src/winapp-CLI/WinApp.Cli/Services/YamlPackagesHasher.cs | Adds stable hashing for packages: block for lockfile staleness checks. |
| src/winapp-CLI/WinApp.Cli/Services/WinmdsLockfileService.cs | Adds best-effort read/write of .winapp/winmds.lock.json with schema validation and atomic writes. |
| src/winapp-CLI/WinApp.Cli/Services/UserPackageJsonService.cs | Adds safe editing of user package.json to ensure runtime deps are production deps. |
| src/winapp-CLI/WinApp.Cli/Services/PackageManagerDetector.cs | Detects npm/yarn/pnpm/bun for tailored install hints. |
| src/winapp-CLI/WinApp.Cli/Services/NpmWrapperVersionProvider.cs | Locates npm wrapper’s pinned versions by walking up from the native exe. |
| src/winapp-CLI/WinApp.Cli/Services/JsBindingsPresets.cs | Adds presets + per-package emit/ref/skip classification and winmd partitioning helpers. |
| src/winapp-CLI/WinApp.Cli/Services/IWorkspaceSetupService.cs | Minor comment/doc cleanup. |
| src/winapp-CLI/WinApp.Cli/Services/IWinmdsLockfileService.cs | New interface for winmd lockfile read/write. |
| src/winapp-CLI/WinApp.Cli/Services/IUserPackageJsonService.cs | New interface + outcome enum for package.json injection. |
| src/winapp-CLI/WinApp.Cli/Services/IPackageManagerDetector.cs | New interface for package manager detection. |
| src/winapp-CLI/WinApp.Cli/Services/INpmWrapperVersionProvider.cs | New interface for wrapper-version resolution. |
| src/winapp-CLI/WinApp.Cli/Services/IJsBindingsWorkspaceService.cs | New orchestration interface + context/result models for bindings pipeline. |
| src/winapp-CLI/WinApp.Cli/Services/IDynWinrtCodegenService.cs | New interface for invoking dynwinrt-codegen. |
| src/winapp-CLI/WinApp.Cli/Services/IConfigService.cs | Adds SaveJsBindingsOnly to splice jsBindings while preserving other YAML. |
| src/winapp-CLI/WinApp.Cli/Services/ConfigService.cs | Extends YAML parsing/serialization to support jsBindings: and splicing logic. |
| src/winapp-CLI/WinApp.Cli/Models/WinmdsLockfile.cs | Adds lockfile model + source-gen JSON context. |
| src/winapp-CLI/WinApp.Cli/Models/WinappConfig.cs | Adds JsBindings optional config block to the main config model. |
| src/winapp-CLI/WinApp.Cli/Models/JsBindingsConfig.cs | New model for user-facing jsBindings settings (lang/output/packages/extraTypes/refs). |
| src/winapp-CLI/WinApp.Cli/Helpers/HostBuilderExtensions.cs | Registers new services/commands in DI and command tree. |
| src/winapp-CLI/WinApp.Cli/Commands/WinAppRootCommand.cs | Adds node top-level command category/registration. |
| src/winapp-CLI/WinApp.Cli/Commands/NodeCommand.cs | New node command host for npm-only subtrees. |
| src/winapp-CLI/WinApp.Cli/Commands/JsBindingsCommand.cs | New node jsbindings command host with js-bindings alias. |
| src/winapp-CLI/WinApp.Cli/Commands/InitCommand.cs | Adds --js-bindings* flags, preset alias flags, and npm-only gating. |
| src/winapp-CLI/WinApp.Cli/Commands/GenerateJsBindingsCommand.cs | Implements node jsbindings generate (read-only yaml + codegen). |
| src/winapp-CLI/WinApp.Cli/Commands/AddJsBindingsCommand.cs | Implements node jsbindings add (mutates yaml + codegen) with presets/force/no-prompt logic. |
| src/winapp-CLI/WinApp.Cli.Tests/YamlPackagesHasherTests.cs | Unit tests for hash stability/case/ordering behavior. |
| src/winapp-CLI/WinApp.Cli.Tests/WorkspaceSetupServiceTests.cs | Adds propagation tests for the jsBindings step within workspace setup. |
| src/winapp-CLI/WinApp.Cli.Tests/WinmdsLockfileServiceTests.cs | Unit tests for lockfile schema/atomic write/read behavior and categorization. |
| src/winapp-CLI/WinApp.Cli.Tests/UserPackageJsonServiceTests.cs | Unit tests for package.json injection outcomes + formatting preservation. |
| src/winapp-CLI/WinApp.Cli.Tests/TestDoubles/FakeJsBindingsWorkspaceService.cs | Test double for orchestrator wiring tests. |
| src/winapp-CLI/WinApp.Cli.Tests/TestDoubles/FakeDynWinrtCodegenService.cs | Test double for codegen invocation without spawning binaries. |
| src/winapp-CLI/WinApp.Cli.Tests/PackageManagerDetectorTests.cs | Unit tests for package manager detection precedence. |
| src/winapp-CLI/WinApp.Cli.Tests/NpmWrapperVersionProviderTests.cs | Unit tests for wrapper-version lookup and failure messaging. |
| src/winapp-CLI/WinApp.Cli.Tests/JsBindingsPresetsTests.cs | Unit tests for presets, classification, partitioning, and scope demotion behavior. |
| src/winapp-CLI/WinApp.Cli.Tests/GenerateJsBindingsCommandTests.cs | Command-tree and behavior tests for node jsbindings generate. |
| src/winapp-CLI/WinApp.Cli.Tests/DynWinrtCodegenStagingTests.cs | Tests staging/swap semantics for safe output replacement. |
| src/winapp-CLI/WinApp.Cli.Tests/DynWinrtCodegenOutputSafetyTests.cs | Tests output-dir safety gates (marker/reparse-point/escape rejection). |
| src/winapp-CLI/WinApp.Cli.Tests/DynWinrtCodegenInvocationTests.cs | Tests executable resolution and safe PATH lookup behaviors. |
| src/winapp-CLI/WinApp.Cli.Tests/DynWinrtCodegenArgvTests.cs | Tests argv building and helper list-merge/dedup logic. |
| src/winapp-CLI/WinApp.Cli.Tests/BaseCommandTests.cs | Adds test DI stub for INpmWrapperVersionProvider to avoid layout walking. |
| scripts/generate-llm-docs.ps1 | Updates schema version normalization and skill-command mapping. |
| samples/electron/test.Tests.ps1 | Adds end-to-end smoke tests for jsBindings add flow in Electron sample. |
| README.md | Adds node jsbindings add to root command list. |
| docs/usage.md | Documents init jsBindings flags and node jsbindings add. |
| docs/npm-usage.md | Updates generated npm API docs with new init flags and node jsbindings APIs. |
| docs/guides/electron/index.md | Adds Electron guide path for dynwinrt JS bindings option. |
| docs/fragments/skills/winapp-cli/setup.md | Adds skill snippet describing jsBindings usage. |
| docs/fragments/skills/winapp-cli/frameworks.md | Adds framework guidance for jsBindings in Electron/npm usage. |
| docs/cli-schema.json | Updates CLI schema to include new flags and node jsbindings command tree. |
| .gitignore | Ignores local .claude/ directory. |
| .github/plugin/skills/winapp-cli/setup/SKILL.md | Updates generated skill doc content (includes new init flags and unregister section). |
| .github/plugin/skills/winapp-cli/frameworks/SKILL.md | Updates generated frameworks skill doc with jsBindings guidance. |
| .github/plugin/agents/winapp.agent.md | Updates agent decision tree with jsBindings guidance for Electron. |
Files not reviewed (1)
- src/winapp-npm/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Build Metrics ReportBinary Sizes
Test Results✅ 1065 passed, 1 skipped out of 1066 tests in 420.5s (+36 tests, -37.1s vs. baseline) Test Coverage❌ 16.8% line coverage, 34.5% branch coverage · ✅ +0.1% vs. baseline CLI Startup Time45ms median (x64, Updated 2026-05-27 05:50:24 UTC · commit |
nmetulev
left a comment
There was a problem hiding this comment.
🤖 Automated PR review by GitHub Copilot CLI (
pr-reviewskill, Claude Opus 4.7 orchestrator + GPT-5.4 multi-model cross-check). Findings below are AI-generated suggestions; please verify before acting.
PR Review — leilzh/jsbindings vs origin/main (6 commits, 70 files, +10705/-157)
⚠️ This PR exceeds the 50-file soft guardrail. Full diff (~560 KB) was reviewed end-to-end across all 8 dimensions, but the sheer surface area means individual hunks got proportionally less attention than they would in a smaller PR.
Summary
Critical: 0 High: 1 Medium: 10 Low: 0 (plus 1 candidate-high dropped by multi-model cross-check — see below)
Coverage
| Dimension | Result |
|---|---|
| security | ⚠ 2 findings |
| correctness | ⚠ 3 findings |
| cli-ux | ⚠ 3 findings (1 disputed by cross-check → dropped) |
| alternative-solution | ⚠ 2 findings |
| test-coverage | ⚠ 2 findings |
| docs-and-samples | ⚠ 1 finding |
| packaging | ✓ clean |
| multi-model | ✓ 1/3 high confirmed, 1 downgraded to medium, 1 disputed; no new criticals/highs |
Findings
| ID | Location | Domain | Summary |
|---|---|---|---|
| H1 | DynWinrtCodegenService.cs:510-558 |
security | Workspace-local node_modules/@microsoft/dynwinrt-codegen is preferred over the wrapper-bundled copy → a cloned repo can replace the binary winapp launches |
| M1 | ConfigService.cs:44-560 |
alternative-solution | YAML splicer/parser/renderer in ConfigService should be a WinappConfigDocument-style data type (cf. AppxManifestDocument) |
| M2 | ConfigService.cs:454-459 |
correctness | TryReadScalar keeps inline # comments in the value — output: bindings/winrt # generated parses as a literal #-suffixed path |
| M3 | DynWinrtCodegenService.cs:54-60 |
cli-ux | Resolved executable path logged at Information level; should be debug-only |
| M4 | JsBindingsWorkspaceService.cs:15-906 |
alternative-solution | 906-line "god service" with 11 injected deps mixes orchestration, discovery, config mutation, prompting, and package.json edits — over the soft size limit |
| M5 | JsBindingsWorkspaceService.cs:329-345 |
test-coverage | UNC hardening for additionalRefs and lockfile PartitionFromLockfile paths is uncovered (only additionalWinmds UNC is tested) |
| M6 | JsBindingsWorkspaceService.cs:388-409 |
correctness | Live NuGet dependency expansion swallows all exceptions silently → incomplete metadata graph can produce truncated bindings with no user warning |
| M7 | JsBindingsWorkspaceService.cs:571-583 |
correctness | Best-effort package.json update only catches InvalidOperationException; raw IOException/UnauthorizedAccessException from UserPackageJsonService will abort the command instead of degrading to a warning (downgraded from high) |
| M8 | JsBindingsWorkspaceService.cs:767-797 |
test-coverage | Overlap-aware old-output cleanup (the guard against --force --output deleting freshly-generated bindings) has no orchestration test |
| M9 | UserPackageJsonService.cs:24-85 |
security | EnsureRuntimeDependency writes package.json without rejecting reparse points → a malicious workspace can redirect the rewrite via a symlink/junction |
| M10 | src/winapp-npm/src/cli.ts:202-214 |
cli-ux | showCombinedHelp() is stale: doesn't list node jsbindings add / node jsbindings generate, so the new flow isn't discoverable from winapp --help |
| M11 | src/winapp-VSC/package.json:18-120 |
docs-and-samples | VS Code extension doesn't surface the new node jsbindings commands (or document the intentional product boundary) |
Dropped after multi-model cross-check
H3 (cli-ux):Disputed → dropped. Independent re-read shows--config-dirdoesn't retarget the workspace fornode jsbindings add.base-directory(workspace) and--config-dir(location ofwinapp.yaml) are intentionally separable;AddJsBindings_WithConfigDirSeparateFromWorkspace_PatchesIntendedYamllocks this in, and the docs describe it that way. This is at most a docs/ergonomics conversation, not a correctness bug.
Details
H1 src/winapp-CLI/WinApp.Cli/Services/DynWinrtCodegenService.cs:510-558
- Severity: high · Confidence: high · Domain: security · Multi-model: confirmed
- Finding:
ResolveCodegenInvocationexecutes a workspace-localnode_modules/@microsoft/dynwinrt-codegenbefore the wrapper-bundled copy, so a cloned repo (or anything that drops a fake under the user's workspace) can replace the binarywinapplaunches. - Evidence: Lines 510-514 explicitly prioritize a repo-controlled install (
// Search workspace ancestry first (user-installed override)...,var roots = new List<DirectoryInfo> { workspaceDir };). Lines 533-558 return and execute that package immediately. The dedicated testResolveCodegenInvocation_InnerNodeModulesShadowsOuterlocks in the workspace-local precedence. - Recommendation: Resolve codegen from the wrapper's own pinned install first (or exclusively), and only honor a workspace override behind an explicit opt-in flag with version/integrity validation.
M1 src/winapp-CLI/WinApp.Cli/Services/ConfigService.cs:44-560
- Severity: medium · Confidence: high · Domain: alternative-solution
- Finding:
ConfigServicenow owns a full custom YAML parser/splicer/renderer forjsBindings—SaveJsBindingsOnly(),Parse()/ParseJsBindingsLine(),AppendJsBindingsBlock(),SpliceJsBindingsBlock(). That's a data-document responsibility, not file I/O. - Evidence: Lines 44-65 (splice), 172-452 (parse), 513-560 (render) all live inside the service.
- Recommendation: Extract
winapp.yamlstructure handling into aWinappConfigDocument/JsBindingsConfigDocument(mirroringAppxManifestDocument), keepingConfigServiceas a thin load/save wrapper. Otherwise this file becomes the long-term home for ad-hoc YAML grammar.
M2 src/winapp-CLI/WinApp.Cli/Services/ConfigService.cs:454-459
- Severity: medium · Confidence: high · Domain: correctness
- Finding:
TryReadScalaronly strips leadingprefixand surrounding quotes; an unquoted trailing# commentis kept as part of the value. Sooutput: bindings/winrt # generated filesparses asbindings/winrt # generated files. - Evidence:
value = t.Substring(prefix.Length).Trim().Trim('"', '\'');(lines 454-459).ParseJsBindingsLineroutes bothlangandoutputthrough this helper (lines 268-269). - Recommendation: Strip everything from the first unquoted
#(and apply the same rule to list items) before trimming quotes; add a regression test with a commented scalar.
M3 src/winapp-CLI/WinApp.Cli/Services/DynWinrtCodegenService.cs:54-60
- Severity: medium · Confidence: high · Domain: cli-ux
- Finding: Default-verbosity runs print the resolved
dynwinrt-codegenexecutable atInformation, exposing internalnode_modules/ binary paths that should be debug-only (and which already get added totaskContext.AddDebugMessageon the next line). - Evidence:
logger.LogInformation("Resolved dynwinrt-codegen → {Executable} {PrefixArgs}", ...)(lines 54-60). - Recommendation: Downgrade to
LogDebug; the debug-message line below already preserves it for--verbose.
M4 src/winapp-CLI/WinApp.Cli/Services/JsBindingsWorkspaceService.cs:15-906
- Severity: medium · Confidence: high · Domain: alternative-solution
- Finding: 906-line service with 11 constructor-injected dependencies combines orchestration (
RunAsync,AddAsync,GenerateAsync), winmd discovery (ResolveAdditionalWinmds), interactive prompting, runtime dependency editing (EnsureRuntimeDependencyAndPrintHint), and config mutation. Well over the repo's soft ~800-line limit and the "one responsibility" guideline in the agent docs. - Evidence: Lines 15-27 (11 deps), 29-155 (
RunAsync), 415-493 (winmd discovery), 554-613 (runtime dep), 615-809 (AddAsync), 811-880 (GenerateAsync). - Recommendation: Keep top-level orchestration here; extract winmd-discovery + runtime-dependency editing + config-prompt path into separate collaborators (or partials). Brings the file back under soft limit and narrows the surface for future changes.
M5 src/winapp-CLI/WinApp.Cli/Services/JsBindingsWorkspaceService.cs:329-345
- Severity: medium · Confidence: high · Domain: test-coverage
- Finding: UNC/network-path hardening is only half-tested: there's a test for
additionalWinmdsUNC rejection, but no companion foradditionalRefs, andPartitionFromLockfile's UNC drop branch has no unit test either. - Evidence: Production code drops UNC paths in both emit and ref sets (
if (IsNetworkPath(path)) continue;in lockfile partition,jsBindings.{FieldName} entry refusedforadditionalRefs/additionalWinmds). Coverage inAddJsBindingsOrchestrationTestsonly exercisesadditionalWinmds;WinmdsLockfileServiceTestsdoesn't feed UNC paths throughPartitionFromLockfile. - Recommendation: Add (a) an orchestration test with
jsBindings.additionalRefspointing at a UNC path, (b) a unit test feeding UNC paths throughPartitionFromLockfilefor both emit and ref-only packages, asserting they're dropped before anyFileInfoprobing.
M6 src/winapp-CLI/WinApp.Cli/Services/JsBindingsWorkspaceService.cs:388-409
- Severity: medium · Confidence: high · Domain: correctness
- Finding: Live-mode transitive expansion swallows every NuGet lookup exception silently and continues with a partial dependency graph; downstream
packageLayoutService.FindWinmds(usedVersions)then generates bindings from incomplete metadata with no user-visible warning. - Evidence:
var deps = await nugetService.GetPackageDependenciesAsync(...)wrapped bycatch (Exception ex) { taskContext.AddDebugMessage(...); logger.LogDebug(...); }(lines 390-409), feeding intoLiveDiscoveryAsyncat 254-267. - Recommendation: Surface transitive-expansion failures as a user-visible warning/error (or fail the slow path with "run
winapp restorefirst"), rather than producing silently truncated bindings.
M7 src/winapp-CLI/WinApp.Cli/Services/JsBindingsWorkspaceService.cs:571-583
- Severity: medium · Confidence: high · Domain: correctness · Multi-model: downgrade from high (real bug, requires abnormal FS state to trigger)
- Finding:
EnsureRuntimeDependencyAndPrintHintonly catchesInvalidOperationException, butUserPackageJsonService.EnsureRuntimeDependencyperforms rawFile.OpenRead/File.ReadAllText/File.WriteAllTextthat can throwIOException/UnauthorizedAccessException. A locked or read-onlypackage.jsonwill abortinit --js-bindings/node jsbindings *instead of degrading to the documented manual-install warning. - Evidence: Caller catches
InvalidOperationExceptiononly (lines 571-583); callee raw I/O atUserPackageJsonService.cs:35-36,79-85. Same exception bubbles out of both init and post-codegen paths. - Recommendation: Wrap filesystem exceptions inside
UserPackageJsonService.EnsureRuntimeDependency(return a failure outcome), or broaden the caller's catch to includeIOException/UnauthorizedAccessException. Don't blanket-catchException.
M8 src/winapp-CLI/WinApp.Cli/Services/JsBindingsWorkspaceService.cs:767-797
- Severity: medium · Confidence: high · Domain: test-coverage
- Finding: The overlap-aware old-output cleanup — the guard that prevents
--force --outputfrom deleting freshly generated bindings when old/new directories nest — has no orchestration test. - Evidence: Code now skips cleanup when
IsNestedPathis true in either direction and logs"Previous bindings dir {OldDir} overlaps new output {NewDir}; skipping cleanup.". Orchestration tests inAddJsBindingsOrchestrationTests.csonly use disjoint dirs (managed-old/fresh-out,unmanaged-old/fresh-out-2). - Recommendation: Add cases for both nesting directions (
bindings→bindings/winrtand the reverse), asserting cleanup is skipped and freshly generated files survive.
M9 src/winapp-CLI/WinApp.Cli/Services/UserPackageJsonService.cs:24-85
- Severity: medium · Confidence: high · Domain: security
- Finding:
EnsureRuntimeDependencybuildsworkspaceDirectory + "package.json", checks existence, then unconditionallyFile.OpenRead/File.WriteAllTexts — no reparse-point or root-containment check. A malicious workspace can redirect the automatic dependency rewrite via a symlink/junction to apackage.jsonoutside the workspace. - Evidence: Lines 35-36 (
File.OpenRead) and 79-85 (File.WriteAllText) with onlyFile.Existsbetween them. - Recommendation: Reject the rewrite when
FileInfo(packageJsonPath).Attributes.HasFlag(FileAttributes.ReparsePoint)(or any ancestor is a reparse point), or canonicalize and require the resolved path to stay inside the workspace root.
M10 src/winapp-npm/src/cli.ts:202-214
- Severity: medium · Confidence: high · Domain: cli-ux
- Finding:
showCombinedHelp()is stale: it still lists onlynode create-addon,node add-electron-debug-identity,node clear-electron-debug-identity. The newwinapp node jsbindings add/generateflow only appears in the nestedwinapp nodehelp, not in the top-levelwinapp --helplisting. - Evidence: Lines 205-214 of
src/winapp-npm/src/cli.ts(only the older subcommands). Nested help was updated in the diff butshowCombinedHelp()wasn't. - Recommendation: Add the new subcommands and an example to
showCombinedHelp()so the new flow is discoverable from the main entry point.
M11 src/winapp-VSC/package.json:18-120
- Severity: medium · Confidence: medium · Domain: docs-and-samples
- Finding: VS Code extension doesn't surface the new
winapp node jsbindingscommands inactivationEvents/contributes.commands. Either (a) extension parity is missing, or (b) this is intentionally npm-only and should be documented. - Evidence: Manifest stops at the existing surface (
onCommand:winapp.certInfo,"command": "winapp.certInfo", "title": "Certificate Info"); nojsbindingsentries. - Recommendation: Add Command Palette entries for the JS bindings flows under
"category": "WinApp", or add a short note in the extension README explaining the intended product boundary.
🤖 Generated by the pr-review skill in GitHub Copilot CLI. Orchestrator: Claude Opus 4.7. Specialist dimensions: security, correctness, cli-ux, alternative-solution, test-coverage, docs-and-samples, packaging. Multi-model cross-check: GPT-5.4.
nmetulev
left a comment
There was a problem hiding this comment.
- Should we have
winapp initdetect when called from npm package and show an option to the user to setup C++, JS, or both bindings? I feel that would simplify the process and remove the need for thejsbindingscommands which can be removed - we can simply usewinapp initandwinapp restorelike we do for c++ - The
--js-bindings-aifeels odd, why are we adding a single property just for AI and not for anything else? Could we do without this option?
This comment was marked as outdated.
This comment was marked as outdated.
nmetulev
left a comment
There was a problem hiding this comment.
- Should move the question for bindings towards the end because as a first question it is not clear what JS/TS bindings we are asking the user to add, and if they decide not to setup sdks, js bindings can't be added:
- Should update guides/electron/setup.md step 3 to include the js bindings steps. Also add a link to the js-binding.md guide at the end for next steps. Maybe also update the other electron guides that use native bindings to point to this new js bindings - though we should maybe recomend this approach over the others to avoid having too many options for users?
| npm install # materializes the @microsoft/dynwinrt runtime dep | ||
| ``` | ||
|
|
||
| ### Path B — Existing project (layer bindings on) |
There was a problem hiding this comment.
I'm wondering if we should simplify this to only path A - even if it's an existing project, npx winapp init could still be used to add js bindings, no?
| - Completed the [development environment setup](setup.md) | ||
| - Used `winapp` via `npx` (the `@microsoft/winappcli` npm package) — JS bindings only work through the npm shim; the standalone winget / installer build doesn't surface them. | ||
|
|
||
| ## Step 1: Add JS bindings to your project |
There was a problem hiding this comment.
Should this just move to the setup doc when they call init for the first time?
| Both paths produce a `bindings/` directory next to your sources: | ||
|
|
||
| ``` | ||
| bindings/ |
There was a problem hiding this comment.
why not place the bindings by default in the .winapp folder? it's is already gitignored and it's where the C++ bindings also go.
| > [!NOTE] | ||
| > **Which import syntax should I use?** Check your `package.json`: | ||
| > | ||
| > - **Has a bundler plugin** (`@electron-forge/plugin-vite`, `@electron-forge/plugin-webpack`, etc. — this is the **current `electron-forge` default**): use the top-level `import` shown above as-is. Done. |
| ```jsonc | ||
| // package.json | ||
| { | ||
| "winapp": { |
There was a problem hiding this comment.
I like that we are using package.json here - I'm wondering if we could replace winapp.yaml with this entry also - not as part of this PR, but as a separate work item - do you think it's possible?
| > ```js | ||
| > const { FileOpenPicker, PickerLocationId, PickerViewMode } = await import('../bindings/index.js'); | ||
| > ``` | ||
| > (Top-level `await` doesn't work in CommonJS, so the call must be inside `async`.) |
|
|
||
| > Why production not devDep? `@microsoft/dynwinrt` is the runtime that powers the generated bindings — without it, your generated `bindings/*.js` files fail to load at runtime. It's not a build-only tool. | ||
|
|
||
| ## How it works under the hood |
There was a problem hiding this comment.
Do we need all of this documentation in this guide? this is supposed to be a step by step guide. would it make more sense to link to have separate documentation to link to that goes into all of these details?
| "nan": "^2.24.0", | ||
| "node-addon-api": "^8.5.0", | ||
| "node-gyp": "^12.1.0" | ||
| }, |
There was a problem hiding this comment.
should the sample be updated to show js bindings usage? this way we also test the bindings on each pr to ensure we catch issues
PR Review —
|
| Dimension | Result |
|---|---|
| security | ✓ clean |
| correctness | ⚠ 2 findings |
| cli-ux | ⚠ 2 findings |
| alternative-solution | ⚠ 2 findings |
| test-coverage | ⚠ 2 findings |
| docs-and-samples | ⚠ 1 finding |
| packaging | ✓ clean |
| multi-model | ✓ 2/2 high confirmed |
Findings
| ID | Location | Domain | Summary |
|---|---|---|---|
| H1 | src/winapp-npm/src/jsbindings/orchestrator.ts:63-74 |
correctness | restore regresses native no-op contract when lockfile is missing |
| H2 | src/winapp-npm/src/jsbindings/orchestrator.ts:177-202 |
cli-ux | Runtime-dependency injection failure still exits 0 |
| M1 | scripts/generate-llm-docs.ps1 (frameworks skill desc) |
docs-and-samples | Skill description omits JS bindings as Electron path |
| M2 | samples/electron/test.Tests.ps1:311-342 |
test-coverage | Phase 2 doesn't exercise new JS-bindings restore/generate path |
| M3 | src/winapp-npm/package.json:10-30 |
test-coverage | No npm-side unit tests for ~2.8k lines of new TS modules |
| M4 | src/winapp-npm/src/cli.ts:647-730 |
cli-ux | Wrapper hooks don't honor --json (corrupts machine output) |
| M5 | src/winapp-npm/src/jsbindings/codegen-runner.ts:253-260 |
correctness | CherryPick entries w/o winmdPath defeat SDK auto-detection of Windows.winmd |
| L1 | src/winapp-CLI/WinApp.Cli/Services/WinmdsLockfileService.cs:179-192 |
alternative-solution | Re-parses NuGet cache layout instead of reusing PackageLayoutService |
| L2 | src/winapp-npm/src/jsbindings/package-json-doc.ts:13-16 |
alternative-solution | New helper not adopted by existing cs/cpp-addon-utils mutation sites |
Details
H1 src/winapp-npm/src/jsbindings/orchestrator.ts:63-74
- Severity: high · Confidence: high · Multi-model: confirmed
- Finding:
restoreno longer preserves the native no-op contract whenpackage.jsonhaswinapp.jsBindingsbut no lockfile exists. - Evidence:
handleRestoreinsrc/winapp-npm/src/cli.ts:744-749always invokes the JS codegen orchestrator after native restore. The orchestrator turns a missing lockfile intolockfileMissing(orchestrator.ts:63-74) before reaching the "nothing to generate" path at lines 139-145. Native restore explicitly returns success for both "no winapp.yaml" and "winapp.yaml has no packages" (WorkspaceSetupService.cs:81-85,883-887), so the wrapper regresses that no-op contract into a failure exit. - Recommendation: Treat a missing lockfile as an empty package inventory when restore had no packages; only fail when NuGet-backed bindings are actually configured and require the lockfile.
H2 src/winapp-npm/src/jsbindings/orchestrator.ts:177-202
- Severity: high · Confidence: high · Multi-model: confirmed
- Finding: Failure to add the required runtime dependency exits successfully even though generated bindings may not run.
- Evidence: Lines 177-195 swallow injector exceptions with a
⚠️ Failed to ensure runtime dependency...log; lines 198-202 still returnoutcome: 'completed', whichcli.ts:762-766maps to a green success message and exit 0. Cross-check note: broader than thrown exceptions —ensureRuntimeDependencycan also return non-satisfying outcomes (presentInDevDependencies,noPackageJson) while the orchestrator still reports success. - Recommendation: Fail the command (non-zero exit) on any injector outcome that does not guarantee a usable production dependency — not just on thrown errors. Consider a distinct failure outcome the CLI maps to exit 1.
M1 scripts/generate-llm-docs.ps1 (frameworks skill description, ~line 250)
- Severity: medium · Confidence: high
- Finding: The shipped
frameworksskill description does not surface JS bindings as a key Windows-API path for Electron/Node, so agent matching may miss the new workflow. - Evidence: The description reads "Framework-specific Windows development guidance for Electron, .NET (WPF, WinForms), C++, Rust, Flutter, and Tauri." It does not mention JS bindings even though
docs/guides/electron/js-bindings.md(432 lines) andnode generate-bindingsship in this skill's scope. - Recommendation: Extend the
frameworksskill description in$SkillDescriptionsto call out JS bindings explicitly (e.g., "...including JS bindings (typed WinRT for Node/Electron) and native addons") and regenerate viascripts/build-cli.ps1.
M2 samples/electron/test.Tests.ps1:311-342
- Severity: medium · Confidence: high
- Finding: Phase 2 of the Electron sample test does not exercise the new JS-bindings restore/generate path the sample was just wired up to use.
- Evidence:
samples/electron/package.jsonnow declares"winapp": { "jsBindings": {} }andpostinstall: "winapp restore ..."(lines ~19, 43-45), but Phase 2 runsnpm install --ignore-scriptsand only builds the C# addon, never invoking the bindings pipeline. - Recommendation: In Phase 2, run
npx winapp restore(ornpx winapp node generate-bindings) and assert the generated artifacts exist (e.g.,bindings/.dynwinrt-managed,bindings/index.js).
M3 src/winapp-npm/package.json:10-30
- Severity: medium · Confidence: high
- Finding: ~2,800 lines of new TS jsbindings code ship with zero npm-side unit tests; the package has no
testscript. - Evidence: Scripts include
compile,lint,buildbut notest. Several new files are pure-logic and trivially unit-testable:yaml-packages-hash.ts(242),path-safety.ts(183),lockfile-reader.ts(199),package-manager-detector.ts(93),cli-args.ts(107). - Recommendation: Add a lightweight TS test runner (vitest/node:test) and cover the pure-logic modules above plus the CLI arg parser. Wire it into CI.
M4 src/winapp-npm/src/cli.ts:647-730
- Severity: medium · Confidence: high
- Finding: New
init/restorewrapper hooks honor--quietbut not--json, so wrapper status text and spinner output can corrupt machine-readable output. - Evidence:
const quiet = isQuiet(args);at 649 and conditionalconsole.log(...)at 668-670 are gated only onquiet;runJsBindingsOrchestrator(...)is invoked at 730 without any JSON-mode suppression of spinners/prompts/logs. - Recommendation: Parse
--jsonalongside--quiet; in JSON mode, suppress all wrapper logs, disable the spinner, skip interactive prompts (or fail fast), and ensure no stdout interleaving with the native CLI's JSON output.
M5 src/winapp-npm/src/jsbindings/codegen-runner.ts:253-260
- Severity: medium · Confidence: medium
- Finding: For
CodegenCherryPickentries that don't supplywinmdPath, passing the union of emit winmds via--winmdprevents dynwinrt-codegen from auto-detecting the SDK'sWindows.winmd. - Evidence:
const emitSet = new Set<string>(emitWinmds); ... if (emitSet.size > 0) { args.push('--winmd', Array.from(emitSet).join(';')); }. Once--winmdis supplied, auto-detection is bypassed, so namespace-only cherry-picks lose access to Windows types. - Recommendation: For path-less cherry-pick entries, either omit
--winmdand let auto-detection run, or explicitly include the SDKWindows.winmdpath in the set.
L1 src/winapp-CLI/WinApp.Cli/Services/WinmdsLockfileService.cs:179-192
- Severity: low · Confidence: high
- Finding: The new service re-parses the NuGet cache layout (
<cache>/<package-id-lowercased>/<version>/...) from absolute winmd paths instead of reusing the existing layout owner. - Evidence:
ExtractPackageIdFromPathencodes the layout inline;PackageLayoutService.GetPackageDiralready defines the same convention atsrc/winapp-CLI/WinApp.Cli/Services/PackageLayoutService.cs:11-17. - Recommendation: Extend
PackageLayoutServiceto expose winmds grouped by package id/version (or to reverse-resolve a winmd path to its package), and consume that from the lockfile writer.
L2 src/winapp-npm/src/jsbindings/package-json-doc.ts:13-16
- Severity: low · Confidence: high
- Finding: New
package-json-dochelper is jsbindings-scoped, but existing wrapper sites continue to open-code the same JSON.parse / fs.writeFile flow it warns against. - Evidence: The helper's comment says consumers "should NEVER open-code" package.json I/O, yet
src/winapp-npm/src/cs-addon-utils.ts:327-364andsrc/winapp-npm/src/cpp-addon-utils.ts:212-233still do exactly that. - Recommendation: Either promote this to a shared
package-json-utils.tsand migrate the addon helpers to it, or narrow the helper's contract/comment to jsbindings-only to avoid two parallel patterns.
Coverage notes
- security: Inspected child-process spawning in
codegen-runner.tsandpackage-manager-detector.ts, path-traversal/reparse-point guards in JS+C#PathSafety, lockfile read/write validation, package.json mutation paths, and user-suppliedadditionalWinmds/additionalRefsand WinMD policy handling. No issues found. - packaging: Confirmed new npm dep
@microsoft/dynwinrt-codegen@0.1.0-preview.2is exact-pinned (MIT, win32-only, no audit findings),version.json/VSC versions correctly untouched (npm-only feature), NuGet MSBuild targets unaffected, lockfile service registered lazily, noNoWarn/glob/copyright drift. - multi-model: Re-verified H1 and H2 end-to-end (
cli.ts↔orchestrator.ts↔runtime-dep-injector.ts↔WorkspaceSetupService.cs) and independently rescannedcodegen-runner,lockfile-reader,package-json-config/doc,additional-winmds,winmd-policy,yaml-packages-hash,WinmdsLockfileService,YamlPackagesHasher. Both highs confirmed; no new critical/high findings.
| - **Setup:** `winapp init --use-defaults` → `winapp node create-addon --template cs` (or `--template cpp`) → `winapp node add-electron-debug-identity` | ||
| - **Setup:** `winapp init --use-defaults` → choose your Windows API access path: | ||
| - **JS bindings** — typed `bindings/*.{js,d.ts}` covering the Windows App SDK, called via `@microsoft/dynwinrt` (no native build step). | ||
| - **Add:** `npx winapp init` (interactive prompt) or `npx winapp node generate-bindings` on an existing project. Both write a default `winapp.jsBindings` namespace into `package.json` if missing, then generate. |
There was a problem hiding this comment.
Since you're already mentioning winapp init if the Setup step, I wouldn't re-mention here. Could make the user feel like they have to run it twice.
| - **Setup:** `winapp init --use-defaults` → `winapp node create-addon --template cs` (or `--template cpp`) → `winapp node add-electron-debug-identity` | ||
| - **Setup:** `winapp init --use-defaults` → choose your Windows API access path: | ||
| - **JS bindings** — typed `bindings/*.{js,d.ts}` covering the Windows App SDK, called via `@microsoft/dynwinrt` (no native build step). | ||
| - **Add:** `npx winapp init` (interactive prompt) or `npx winapp node generate-bindings` on an existing project. Both write a default `winapp.jsBindings` namespace into `package.json` if missing, then generate. |
There was a problem hiding this comment.
I don't understand what you mean by this instruction "Both write a default winapp.jsBindings namespace into package.json if missing, then generate." Could you reword?
| - **Setup:** `winapp init --use-defaults` → choose your Windows API access path: | ||
| - **JS bindings** — typed `bindings/*.{js,d.ts}` covering the Windows App SDK, called via `@microsoft/dynwinrt` (no native build step). | ||
| - **Add:** `npx winapp init` (interactive prompt) or `npx winapp node generate-bindings` on an existing project. Both write a default `winapp.jsBindings` namespace into `package.json` if missing, then generate. | ||
| - **Re-run:** `npx winapp node generate-bindings` after editing `winapp.jsBindings.{packages,extraTypes,additionalWinmds}`. Use `npx winapp restore` instead when you changed `winapp.yaml`. |
There was a problem hiding this comment.
Could you reword this to say..."If you need to make edits to winapp.jsBindings, make sure to re-run npx winapp node generate-bindings to regenerate your JS bindings. If you need to make edits to winapp.yaml, make sure run npx winapp restore to update your Windows dependencies.
| - **JS bindings** — typed `bindings/*.{js,d.ts}` covering the Windows App SDK, called via `@microsoft/dynwinrt` (no native build step). | ||
| - **Add:** `npx winapp init` (interactive prompt) or `npx winapp node generate-bindings` on an existing project. Both write a default `winapp.jsBindings` namespace into `package.json` if missing, then generate. | ||
| - **Re-run:** `npx winapp node generate-bindings` after editing `winapp.jsBindings.{packages,extraTypes,additionalWinmds}`. Use `npx winapp restore` instead when you changed `winapp.yaml`. | ||
| - Codegen injects `@microsoft/dynwinrt` as a production dep — run `npm install` afterwards to materialize it. |
There was a problem hiding this comment.
This instruction also feels a bit unclear. Do I have to do this step? Or only in some cases? What do you mean by materialize?
| - The native winapp CLI binary bundled inside `node_modules` | ||
| - A Node.js SDK with helpers for creating native C#/C++ addons | ||
| - Electron-specific commands under `npx winapp node` | ||
| - Typed JS/TypeScript WinRT bindings via dynwinrt (no native build required), opt-in during `npx winapp init` or via `npx winapp node generate-bindings` |
There was a problem hiding this comment.
Since the bindings themselves aren't included, but rather support for generating bindings could you adjust the wording here.
Something like:
"Commands for generating JS/TypeScript WinRT bindings"
I don't this we need to list out commands the actual commands here since this is more of an overview of how the functionality is different.
| npx winapp node add-electron-debug-identity # register identity for debugging | ||
| npx winapp init --use-defaults # fresh init: scaffolds winapp.yaml + JS bindings + C++ projections | ||
| npx winapp node generate-bindings # existing project: add (or re-run) JS bindings only | ||
| npx winapp node create-addon --template cs # create a C# native addon (for what dynwinrt can't drive — see below) |
There was a problem hiding this comment.
I feel like dynwinrt is being mentioned throughout the docs. I know what you mean by that because we're familiar with the package you wrote but if I was a developer reading this for the first time, it wouldn't be obvious to me that you were mentioning a package by name.
I would either change your mentions of dynwinrt to @microsoft/dynwinrt or just leave out mentioning the package by name and instead reference it as "for what the JS bindings can't drive...etc. )
| npx winapp node add-electron-debug-identity # register identity for debugging | ||
| ``` | ||
|
|
||
| #### Choosing between jsBindings and a native addon |
There was a problem hiding this comment.
nit: Can you take a pass through all the doc changes and pick a standard way to write "JS Bindings". I'm seeing "JS Bindings", "JS/Typescript Bindings", "jsBindings" ,etc
| - The native winapp CLI binary bundled inside `node_modules` | ||
| - A Node.js SDK with helpers for creating native C#/C++ addons | ||
| - Electron-specific commands under `npx winapp node` | ||
| - Typed JS/TypeScript WinRT bindings via dynwinrt (no native build required), opt-in during `npx winapp init` or via `npx winapp node generate-bindings` |
There was a problem hiding this comment.
Same notes for this file as the skill.md file
| #### Option A: [Creating a C++ Notification Addon](cpp-notification-addon.md) | ||
| #### Option A: [JS/TypeScript bindings via dynwinrt](js-bindings.md) ✨ *new* | ||
|
|
||
| The simplest path — typed JS/TypeScript wrappers generated from `.winmd` metadata, no native build step required from your Electron project. Opt in during `npx winapp init` (or pass `--use-defaults` to auto-accept) and a `bindings/` directory is dropped next to your sources. You `import { ChatClient } from './bindings'` and call WinRT directly. |
There was a problem hiding this comment.
"dropped next your sources" feels like odd phrasing. Could you change to "added to your project"?
| #### Option A: [Creating a C++ Notification Addon](cpp-notification-addon.md) | ||
| #### Option A: [JS/TypeScript bindings via dynwinrt](js-bindings.md) ✨ *new* | ||
|
|
||
| The simplest path — typed JS/TypeScript wrappers generated from `.winmd` metadata, no native build step required from your Electron project. Opt in during `npx winapp init` (or pass `--use-defaults` to auto-accept) and a `bindings/` directory is dropped next to your sources. You `import { ChatClient } from './bindings'` and call WinRT directly. |
There was a problem hiding this comment.
Can you change "You import ..." to "You can then add import..."
|
|
||
| [Add JS bindings →](js-bindings.md) | ||
|
|
||
| > Native addons (Options B–D below) are still the right choice when the API has no WinRT projection — Win32 / pure COM (raw `IFileDialog`, registry, custom COM servers), C++ libraries that ship only headers + a static/shared lib, or vendor SDKs that ship only a managed .NET assembly. For everything that ships in a `.winmd`, jsBindings is the easier on-ramp. |
There was a problem hiding this comment.
JS Bindings are the easier option.
|
|
||
| **JS/TypeScript bindings (npm wrapper only):** | ||
|
|
||
| When run via `npx winapp` in a Node / Electron project, `init` adds an interactive prompt — answering **Y** (the default) writes a `"winapp.jsBindings"` namespace to `package.json` and runs codegen. |
There was a problem hiding this comment.
"...adds an option to generate JS/Typescript bindings, answering Y ..."
|
|
||
| --- | ||
|
|
||
| ### JS/TypeScript bindings (via `init` / `restore`) |
There was a problem hiding this comment.
based on the structure of the rest of this file I think the header here should be the command name like "node generate-bindings"
|
|
||
| ### JS/TypeScript bindings (via `init` / `restore`) | ||
|
|
||
| Declare JS/TS bindings with a `"winapp": { "jsBindings": {...} }` namespace in **`package.json`**. They're generated alongside the workspace lifecycle — no dedicated sub-command. |
There was a problem hiding this comment.
What do you mean my "workspace lifecycle"?
|
|
||
| | Want to … | Command | | ||
| |---|---| | ||
| | Bootstrap a fresh workspace with bindings | `npx winapp init` (answer **Y** at the prompt; default is **Y**) | |
There was a problem hiding this comment.
Same feedback as for the header. For this doc we want to group content by commands rather than by functionality. The init information should be under the init command section instead of here.
| | Want to … | Command | | ||
| |---|---| | ||
| | Bootstrap a fresh workspace with bindings | `npx winapp init` (answer **Y** at the prompt; default is **Y**) | | ||
| | Add bindings to an existing workspace | `npx winapp node generate-bindings` (adds a default `winapp.jsBindings` block on first use) | |
There was a problem hiding this comment.
Also let's make sure this section follows a similar format to the other sections. I would expect to see Options, What it does, and Examples subsections here.
|
|
||
| Before starting this guide, make sure you've: | ||
| - Completed the [development environment setup](setup.md) | ||
| - Used `winapp` via `npx` (the `@microsoft/winappcli` npm package) — JS bindings only work through the npm shim; the standalone winget / installer build doesn't surface them. |
There was a problem hiding this comment.
I would change this to say developers need an Electron project with the @microsoft/winappcli project added as a dev dependency
chiaramooney
left a comment
There was a problem hiding this comment.
@nmetulev just an overall question, are we okay with the init command defaulting to Yes for generating bindings in Electron apps? Or should we default to No?
|
|
||
| ### Path A — Fresh project (init with bindings) | ||
|
|
||
| Run `npx winapp init` and opt in to JS bindings (the interactive prompt defaults to Yes; pass `--use-defaults` to auto-accept in scripted / CI runs). `init` installs the WinAppSDK packages, adds a default `"winapp": { "jsBindings": {} }` namespace to `package.json` (covering the full Windows App SDK), and runs the codegen. |
There was a problem hiding this comment.
I would expand (covering the full Windows App SDK) to instead say.... "When jsBindings has the value {}, bindings are generated for all WinAppSDK APIs"
|
|
||
| ```bash | ||
| npx winapp init --use-defaults | ||
| npm install # materializes the @microsoft/dynwinrt runtime dep |
There was a problem hiding this comment.
Same note as above on the word materializes. Do you mean "install locally"? I would adjust the phrasing.



Description
Call modern Windows Runtime (WinRT) APIs directly from JS or TS inElectron / Node app — no native addon, no node-gyp / MSBuild step, full IntelliSense.
The CLI generates typed .js + .d.ts bindings for WinAppSDK (and any other WinRT) APIs from their .winmd metadata. Bindings call into WinRT at runtime via @microsoft/dynwinrt.
Usage Example
Bootstrap a new project:
Add bindings to an existing project:
Related Issue
Type of Change
Checklist
docs/fragments/skills/(if CLI commands/workflows changed)Screenshots / Demo
Additional Notes
AI Description
This PR introduces typed JavaScript and TypeScript bindings for WinRT APIs in Electron/Node applications, allowing direct API calls without native addons. Developers can now generate these bindings using the CLI with commands such as:
These changes enhance the ability to work with Windows App SDK APIs seamlessly from JavaScript/TypeScript.