Skip to content

Add winapp node jsbindings for typed JS/TS WinRT bindings#536

Open
lei9444 wants to merge 28 commits into
mainfrom
leilzh/jsbindings
Open

Add winapp node jsbindings for typed JS/TS WinRT bindings#536
lei9444 wants to merge 28 commits into
mainfrom
leilzh/jsbindings

Conversation

@lei9444
Copy link
Copy Markdown
Contributor

@lei9444 lei9444 commented May 18, 2026

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:

 npx winapp init --use-defaults    # interactive prompt defaults to Yes
 npm install                       # materializes the @microsoft/dynwinrt runtime dep

Add bindings to an existing project:

 npx winapp node generate-bindings
 npm install

Related Issue

Type of Change

  • ✨ New feature

Checklist

  • New tests added for new functionality (if applicable)
  • Tested locally on Windows
  • Main README.md updated (if applicable)
  • docs/usage.md updated (if CLI commands changed)
  • Language-specific guides updated (if applicable)
  • Sample projects updated to reflect changes (if applicable)
  • Agent skill templates updated in 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:

npx winapp init --use-defaults
npx winapp node generate-bindings

These changes enhance the ability to work with Windows App SDK APIs seamlessly from JavaScript/TypeScript.

Copilot AI review requested due to automatic review settings May 18, 2026 06:45
@github-actions github-actions Bot added the enhancement New feature or request label May 18, 2026
Comment thread src/winapp-npm/scripts/generate-commands.mjs Fixed
Copy link
Copy Markdown
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

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|generate commands and init --js-bindings* flags (npm-only gated via WINAPP_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.

Comment thread README.md Outdated
Comment thread src/winapp-npm/README.md
Comment thread docs/usage.md Outdated
Comment thread scripts/generate-llm-docs.ps1 Outdated
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 18, 2026

Build Metrics Report

Binary Sizes

Artifact Baseline Current Delta
CLI (ARM64) 31.27 MB 31.32 MB 📈 +56.5 KB (+0.18%)
CLI (x64) 31.61 MB 31.67 MB 📈 +55.5 KB (+0.17%)
MSIX (ARM64) 13.12 MB 13.15 MB 📈 +29.8 KB (+0.22%)
MSIX (x64) 13.96 MB 13.99 MB 📈 +29.2 KB (+0.20%)
NPM Package 27.34 MB 27.44 MB 📈 +96.9 KB (+0.35%)
NuGet Package 27.43 MB 27.47 MB 📈 +40.5 KB (+0.14%)
VS Code Extension 20.16 MB 20.20 MB 📈 +43.3 KB (+0.21%)

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 Time

45ms median (x64, winapp --version) · ✅ +7ms vs. baseline


Updated 2026-05-27 05:50:24 UTC · commit c140675 · workflow run

Copy link
Copy Markdown
Member

@nmetulev nmetulev left a comment

Choose a reason for hiding this comment

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

🤖 Automated PR review by GitHub Copilot CLI (pr-review skill, 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): --config-dir doesn't retarget the workspace for node jsbindings add. Disputed → dropped. Independent re-read shows base-directory (workspace) and --config-dir (location of winapp.yaml) are intentionally separable; AddJsBindings_WithConfigDirSeparateFromWorkspace_PatchesIntendedYaml locks 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: ResolveCodegenInvocation executes a workspace-local node_modules/@microsoft/dynwinrt-codegen before the wrapper-bundled copy, so a cloned repo (or anything that drops a fake under the user's workspace) can replace the binary winapp launches.
  • 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 test ResolveCodegenInvocation_InnerNodeModulesShadowsOuter locks 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: ConfigService now owns a full custom YAML parser/splicer/renderer for jsBindingsSaveJsBindingsOnly(), 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.yaml structure handling into a WinappConfigDocument / JsBindingsConfigDocument (mirroring AppxManifestDocument), keeping ConfigService as 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: TryReadScalar only strips leading prefix and surrounding quotes; an unquoted trailing # comment is kept as part of the value. So output: bindings/winrt # generated files parses as bindings/winrt # generated files.
  • Evidence: value = t.Substring(prefix.Length).Trim().Trim('"', '\''); (lines 454-459). ParseJsBindingsLine routes both lang and output through 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-codegen executable at Information, exposing internal node_modules / binary paths that should be debug-only (and which already get added to taskContext.AddDebugMessage on 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 additionalWinmds UNC rejection, but no companion for additionalRefs, and PartitionFromLockfile'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 refused for additionalRefs/additionalWinmds). Coverage in AddJsBindingsOrchestrationTests only exercises additionalWinmds; WinmdsLockfileServiceTests doesn't feed UNC paths through PartitionFromLockfile.
  • Recommendation: Add (a) an orchestration test with jsBindings.additionalRefs pointing at a UNC path, (b) a unit test feeding UNC paths through PartitionFromLockfile for both emit and ref-only packages, asserting they're dropped before any FileInfo probing.

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 by catch (Exception ex) { taskContext.AddDebugMessage(...); logger.LogDebug(...); } (lines 390-409), feeding into LiveDiscoveryAsync at 254-267.
  • Recommendation: Surface transitive-expansion failures as a user-visible warning/error (or fail the slow path with "run winapp restore first"), 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: EnsureRuntimeDependencyAndPrintHint only catches InvalidOperationException, but UserPackageJsonService.EnsureRuntimeDependency performs raw File.OpenRead / File.ReadAllText / File.WriteAllText that can throw IOException / UnauthorizedAccessException. A locked or read-only package.json will abort init --js-bindings / node jsbindings * instead of degrading to the documented manual-install warning.
  • Evidence: Caller catches InvalidOperationException only (lines 571-583); callee raw I/O at UserPackageJsonService.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 include IOException / UnauthorizedAccessException. Don't blanket-catch Exception.

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 --output from deleting freshly generated bindings when old/new directories nest — has no orchestration test.
  • Evidence: Code now skips cleanup when IsNestedPath is true in either direction and logs "Previous bindings dir {OldDir} overlaps new output {NewDir}; skipping cleanup.". Orchestration tests in AddJsBindingsOrchestrationTests.cs only use disjoint dirs (managed-old/fresh-out, unmanaged-old/fresh-out-2).
  • Recommendation: Add cases for both nesting directions (bindingsbindings/winrt and 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: EnsureRuntimeDependency builds workspaceDirectory + "package.json", checks existence, then unconditionally File.OpenRead / File.WriteAllTexts — no reparse-point or root-containment check. A malicious workspace can redirect the automatic dependency rewrite via a symlink/junction to a package.json outside the workspace.
  • Evidence: Lines 35-36 (File.OpenRead) and 79-85 (File.WriteAllText) with only File.Exists between 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 only node create-addon, node add-electron-debug-identity, node clear-electron-debug-identity. The new winapp node jsbindings add / generate flow only appears in the nested winapp node help, not in the top-level winapp --help listing.
  • Evidence: Lines 205-214 of src/winapp-npm/src/cli.ts (only the older subcommands). Nested help was updated in the diff but showCombinedHelp() 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 jsbindings commands in activationEvents / 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"); no jsbindings entries.
  • 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.

Comment thread docs/guides/electron/jsbindings.md Outdated
Comment thread docs/guides/electron/jsbindings.md Outdated
Comment thread docs/guides/electron/jsbindings.md Outdated
Comment thread docs/fragments/skills/winapp-cli/frameworks.md Outdated
@lei9444
Copy link
Copy Markdown
Contributor Author

lei9444 commented May 19, 2026

addressed all comments
image

Copy link
Copy Markdown
Member

@nmetulev nmetulev left a comment

Choose a reason for hiding this comment

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

  • Should we have winapp init detect 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 the jsbindings commands which can be removed - we can simply use winapp init and winapp restore like we do for c++
  • The --js-bindings-ai feels odd, why are we adding a single property just for AI and not for anything else? Could we do without this option?

Comment thread docs/cli-schema.json Outdated
Comment thread docs/cli-schema.json Outdated
Comment thread docs/cli-schema.json Outdated
Comment thread docs/guides/electron/index.md Outdated
Comment thread docs/guides/electron/index.md Outdated
Comment thread docs/guides/electron/jsbindings.md Outdated
Comment thread docs/js-bindings.md Outdated
Comment thread docs/js-bindings.md Outdated
Comment thread src/winapp-CLI/WinApp.Cli/Services/UserPackageJsonService.cs Outdated
Comment thread src/winapp-CLI/WinApp.Cli/Services/DynWinrtCodegenService.cs Outdated
Comment thread src/winapp-CLI/WinApp.Cli/Services/ConfigService.cs Outdated
Comment thread docs/guides/electron/jsbindings.md Outdated
Comment thread src/winapp-CLI/WinApp.Cli/Services/WorkspaceSetupService.Prompts.cs Outdated
@nmetulev

This comment was marked as outdated.

Copy link
Copy Markdown
Member

@nmetulev nmetulev left a comment

Choose a reason for hiding this comment

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

  1. 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:
Image
  1. 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)
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.

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
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.

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/
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.

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.
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.

The default for create-electron-app seems to be commonjs syntax and not vite (shows up as Experimental for me)

Image

```jsonc
// package.json
{
"winapp": {
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.

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`.)
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.

Not sure if I did it right, but I'm getting a warning that there is a performance overhead

Image


> 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
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.

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?

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.

+1

"nan": "^2.24.0",
"node-addon-api": "^8.5.0",
"node-gyp": "^12.1.0"
},
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.

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

@nmetulev
Copy link
Copy Markdown
Member

🤖 AI-generated review. Produced by the Copilot CLI pr-review skill (Claude Opus 4.7 orchestrator + GPT-5.4 multi-model cross-check). Please treat findings as suggestions to verify, not authoritative.

PR Review — leilzh/jsbindings vs origin/main (27 commits, 38 files, +5062/-18)

Summary

Critical: 0 High: 2 Medium: 5 Low: 2

Coverage

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: restore no longer preserves the native no-op contract when package.json has winapp.jsBindings but no lockfile exists.
  • Evidence: handleRestore in src/winapp-npm/src/cli.ts:744-749 always invokes the JS codegen orchestrator after native restore. The orchestrator turns a missing lockfile into lockfileMissing (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 return outcome: 'completed', which cli.ts:762-766 maps to a green success message and exit 0. Cross-check note: broader than thrown exceptions — ensureRuntimeDependency can 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 frameworks skill 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) and node generate-bindings ship in this skill's scope.
  • Recommendation: Extend the frameworks skill description in $SkillDescriptions to call out JS bindings explicitly (e.g., "...including JS bindings (typed WinRT for Node/Electron) and native addons") and regenerate via scripts/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.json now declares "winapp": { "jsBindings": {} } and postinstall: "winapp restore ..." (lines ~19, 43-45), but Phase 2 runs npm install --ignore-scripts and only builds the C# addon, never invoking the bindings pipeline.
  • Recommendation: In Phase 2, run npx winapp restore (or npx 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 test script.
  • Evidence: Scripts include compile, lint, build but no test. 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/restore wrapper hooks honor --quiet but not --json, so wrapper status text and spinner output can corrupt machine-readable output.
  • Evidence: const quiet = isQuiet(args); at 649 and conditional console.log(...) at 668-670 are gated only on quiet; runJsBindingsOrchestrator(...) is invoked at 730 without any JSON-mode suppression of spinners/prompts/logs.
  • Recommendation: Parse --json alongside --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 CodegenCherryPick entries that don't supply winmdPath, passing the union of emit winmds via --winmd prevents dynwinrt-codegen from auto-detecting the SDK's Windows.winmd.
  • Evidence: const emitSet = new Set<string>(emitWinmds); ... if (emitSet.size > 0) { args.push('--winmd', Array.from(emitSet).join(';')); }. Once --winmd is supplied, auto-detection is bypassed, so namespace-only cherry-picks lose access to Windows types.
  • Recommendation: For path-less cherry-pick entries, either omit --winmd and let auto-detection run, or explicitly include the SDK Windows.winmd path 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: ExtractPackageIdFromPath encodes the layout inline; PackageLayoutService.GetPackageDir already defines the same convention at src/winapp-CLI/WinApp.Cli/Services/PackageLayoutService.cs:11-17.
  • Recommendation: Extend PackageLayoutService to 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-doc helper 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-364 and src/winapp-npm/src/cpp-addon-utils.ts:212-233 still do exactly that.
  • Recommendation: Either promote this to a shared package-json-utils.ts and 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.ts and package-manager-detector.ts, path-traversal/reparse-point guards in JS+C# PathSafety, lockfile read/write validation, package.json mutation paths, and user-supplied additionalWinmds/additionalRefs and WinMD policy handling. No issues found.
  • packaging: Confirmed new npm dep @microsoft/dynwinrt-codegen@0.1.0-preview.2 is 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, no NoWarn/glob/copyright drift.
  • multi-model: Re-verified H1 and H2 end-to-end (cli.tsorchestrator.tsruntime-dep-injector.tsWorkspaceSetupService.cs) and independently rescanned codegen-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.
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.

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.
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.

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`.
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.

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.
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.

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`
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.

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)
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.

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
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.

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`
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.

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.
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.

"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.
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.

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.
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.

JS Bindings are the easier option.

Comment thread docs/usage.md

**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.
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.

"...adds an option to generate JS/Typescript bindings, answering Y ..."

Comment thread docs/usage.md

---

### JS/TypeScript bindings (via `init` / `restore`)
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.

based on the structure of the rest of this file I think the header here should be the command name like "node generate-bindings"

Comment thread docs/usage.md

### 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.
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.

What do you mean my "workspace lifecycle"?

Comment thread docs/usage.md

| Want to … | Command |
|---|---|
| Bootstrap a fresh workspace with bindings | `npx winapp init` (answer **Y** at the prompt; default is **Y**) |
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.

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.

Comment thread docs/usage.md
| 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) |
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.

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.
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.

I would change this to say developers need an Electron project with the @microsoft/winappcli project added as a dev dependency

Copy link
Copy Markdown
Contributor

@chiaramooney chiaramooney left a comment

Choose a reason for hiding this comment

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

@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.
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.

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
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.

Same note as above on the word materializes. Do you mean "install locally"? I would adjust the phrasing.

Comment thread src/winapp-npm/src/jsbindings/spinner.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants