Skip to content

fix(types): keep command environments compatible with mobile compilation - #23073

Merged
OrcaWin merged 1 commit into
mainfrom
fix/command-environment-mobile-types
Sep 26, 2026
Merged

OrcaWin merged 1 commit into
mainfrom
fix/command-environment-mobile-types

Conversation

@OrcaWin

@OrcaWin OrcaWin commented Sep 26, 2026 •

Copy link
Copy Markdown
Collaborator
Files Added Deleted Net
Test 0 0 0 0
Prod 1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​2 $\color{#cf222e}{\Huge{\mathbf{−}}}$​2 0

ELI5

Mobile builds were blocked by a shared command helper claiming its environment map always contained Expo's required NODE_ENV field. The helper now declares the map it actually accepts and returns.

What Changed

Change the input and return annotations of mergeCommandEnvironment from NodeJS.ProcessEnv to Record<string, string | undefined>. There is no executable-code change. Desktop keeps its existing environment merge behavior, including case-insensitive Windows keys, and the mobile typecheck succeeds.

Why

Expo augments NodeJS.ProcessEnv with a required NODE_ENV field. A command-specific environment is an arbitrary map and does not promise that field. Using the map's real type avoids a cast or fabricating NODE_ENV, either of which would obscure the contract or change command behavior.

Linked Issue

Follow-up to #22427, exposed by the refreshed #22279 mobile check.

Visual Proof

N/A — type annotations only.

Testing

  • Mobile typecheck reproduced TS2741 before the change and passes after refreshing installed dependencies to the frozen lockfile.
  • Node typecheck passes; existing command-environment suite passes 5 tests, including Windows key folding and unchanged-base behavior.
  • Changed-code quality, focused lint, formatting and whitespace checks pass on macOS.
  • Manual app test; unnecessary for erased type annotations.

Review

One file, two type annotations. No casts, new dependency, environment value or runtime behavior.

Agent skill upstream boundary

  • Not applicable.

Notes

Cross-platform, SSH and folder-workspace runtime behavior is unchanged. No wire change.

Checklist

  • Small, focused change with mechanism and alternatives explained.
  • Relevant tests, typechecks and quality checks passed.
  • Self-reviewed for platform and compatibility impact.

@OrcaWin
OrcaWin merged commit f6eab38 into main Sep 26, 2026
5 checks passed
Jinwoo-H added a commit that referenced this pull request Sep 26, 2026
Picks up #23073 (mobile typecheck fix on main) so CI runs on a current merge ref.

Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ No new issues found.

Reviewed changes

  • Command environment types — mergeCommandEnvironment now declares base and its return as Record<string, string | undefined> instead of NodeJS.ProcessEnv, avoiding Expo's required NODE_ENV augmentation while keeping desktop behavior identical.

The change is type-only, correctly matches the implementation (which returns base or an Object.fromEntries map), and loosens rather than asserts. Verified pnpm tc:node passes and src/shared/command-environment.test.ts passes all 5 tests, including Windows key folding and the unchanged-base case.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant