refactor: move ambient const enums into real modules - #6089
Merged
NathanWalker merged 3 commits intoJul 28, 2026
Merged
Conversation
Copy link
Copy Markdown
Collaborator
Sorry, something went wrong.
Copy link
Copy Markdown
|
Caution Review failedThe pull request is closed. Configuration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 3ed1b666-72a8-43f0-8016-013f935ab4e9 📥 CommitsReviewing files that changed from the base of the PR and between 10ebf46 and f494c86. 📒 Files selected for processing (39)
📝 Walkthrough WalkthroughThis change moves shared enums into runtime-exported modules, updates imports and TypeScript declarations, enables isolated module compilation, standardizes formatting, and changes cleanup request existence checking. Related command, service, analytics, iOS, and test code is aligned with the new enum locations. ChangesEnum migration and consumer alignment
Estimated code review effort: 3 (Moderate) | ~25 minutes Poem 🚥 Pre-merge checks | ✅ 4 | ❌ 1 ❌ Failed checks (1 warning)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. ❤️ ShareComment @coderabbitai help to get the list of available commands. |
Sorry, something went wrong.
Copy link
Copy Markdown
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. Inline comments: In `@lib/detached-processes/cleanup-process.ts`: - Around line 229-249: Update the existence check in removeRequest to compare each queued currentRequestInfo with the removal requestInfo, matching the comparison already used by _.remove. Preserve the existing success and absent-request logging behavior based on whether that matching request is present.
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info ⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c7665e5c-a52a-47d9-aa0e-dfd95c8fc810
📥 CommitsReviewing files that changed from the base of the PR and between 1084bc7 and 10ebf46.
📒 Files selected for processing (38)- lib/commands/build.ts
- lib/commands/debug.ts
- lib/commands/deploy.ts
- lib/commands/prepare.ts
- lib/commands/test.ts
- lib/common/declarations.d.ts
- lib/common/definitions/key-commands.ts
- lib/common/enums.ts
- lib/common/errors.ts
- lib/common/services/analytics/google-analytics-custom-dimensions.ts
- lib/common/services/cancellation.ts
- lib/common/services/commands-service.ts
- lib/common/verify-node-version.ts
- lib/constants.ts
- lib/definitions/project.d.ts
- lib/definitions/system-warnings.d.ts
- lib/definitions/system-warnings.ts
- lib/detached-processes/cleanup-js-subprocess.ts
- lib/detached-processes/cleanup-process.ts
- lib/detached-processes/detached-process-enums.ts
- lib/detached-processes/file-log-service.ts
- lib/helpers/livesync-command-helper.ts
- lib/helpers/options-track-helper.ts
- lib/nativescript-cli.ts
- lib/options.ts
- lib/services/analytics/analytics-broker-process.ts
- lib/services/analytics/analytics-broker.ts
- lib/services/analytics/analytics-service.ts
- lib/services/analytics/google-analytics-cross-client-custom-dimensions.d.ts
- lib/services/analytics/google-analytics-provider.ts
- lib/services/cleanup-service.ts
- lib/services/initialize-service.ts
- lib/services/ios-native-target-service.ts
- lib/services/test-execution-service.ts
- lib/sys-info.ts
- test/options.ts
- test/services/analytics/analytics-service.ts
- test/sys-info.ts
- lib/services/analytics/google-analytics-cross-client-custom-dimensions.d.ts
- lib/definitions/system-warnings.d.ts
Sorry, something went wrong.
Comment thread
lib/detached-processes/cleanup-process.ts
TypeScript inlines `const enum` values at compile time and erases the
import, so declaring them in .d.ts files worked only because tsc has
whole-program type information. Any file-local transpiler (esbuild, swc,
tsx, vite, bun) cannot inline them and fails to resolve the import, which
blocks running the sources without a full tsc pass.
The 12 ambient const enums now live in real modules:
- 5 from lib/common/declarations.d.ts -> lib/common/enums.ts
- BuildNames -> lib/constants.ts, next to the
other build-related enums
- detached-process-enums, system-warnings and
google-analytics-custom-dimensions -> .d.ts renamed to .ts; they held
nothing but enums
- SpecialKeys -> key-commands.d.ts renamed to .ts,
which already exported everything,
so importers are unchanged
The four global files had no imports, making their enums globally visible;
consumers now import them explicitly.
GoogleAnalyticsCrossClientCustomDimensions is referenced nowhere and was
dropped rather than converted into a dead runtime module.
Renaming key-commands to .ts surfaced two interface methods whose missing
return types an ambient context had allowed; both implementations return
nothing, so they are annotated void.
edusperoni
force-pushed
the
refactor/const-enums
branch
from
10ebf46 to
98a0dd8
Compare
July 25, 2026 16:27
Moving the ambient const enums into real modules removed the last thing standing in the way: tsc --noEmit --isolatedModules reports 244 errors on main (all TS2748, 'Cannot access ambient const enums') and none once they are real modules. This is not emit-neutral. With isolatedModules on, tsc stops inlining const enums and emits them as runtime objects, so 33 files change: consumers go from a baked-in literal to a property lookup, and lib/constants.js now emits ShouldMigrate, NativePlatformStatus, DebugTools, TrackActionNames, BuildStates and PlatformTypes. The values are unchanged - verified at runtime - and the suite is unchanged at 1513 passing, with the CLI still rendering help and reporting its version. That change is arguably the point. oxc and esbuild cannot inline a const enum across files, so today tsc's output and every other transpiler's output disagree for exactly those six. Now they agree. It also means the const modifier on the remaining ten declarations no longer buys inlining, so it is decorative; converting them to plain enum is a sensible follow-up.
NathanWalker
approved these changes
Jul 28, 2026
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Reviewers
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Checklist
What is the current behavior?
Twelve const enums are declared inside ambient .d.ts files and used in value positions, for example process.exit(ErrorCodes.UNHANDLED_REJECTION_FAILURE).
That only works because TypeScript has whole-program type information: it inlines the constant at compile time and erases the import entirely. declarations.d.ts produces no runtime module, so there is nothing to import at runtime.
Any file-local transpiler — esbuild, swc, tsx, vite, bun — cannot inline them. It leaves the import in place and resolution fails:
This makes the sources impossible to run through anything other than a full tsc pass, and it is also what isolatedModules disallows.
What is the new behavior?
The twelve ambient const enums now live in real modules:
Four of those files were global ambient declarations with no imports, which is why their enums were visible everywhere without being imported. Consumers now import them explicitly.
Two incidental changes fall out of this:
tsc --noEmit is clean and the suite is unchanged at 1513 passing. Note that the .d.ts files here are hand-written rather than generated (declaration: false), but the package declares no types/typings entry and ships no lib/nativescript-cli-lib.d.ts, so they are not a consumable public type surface.
Summary by CodeRabbit
Bug Fixes
Enhancements
Refactor
Follow-up commit: enable isolatedModules
Moving these enums out of the ambient files removed the last blocker, so the flag is turned on in the same PR:
This part is not emit-neutral, so review it as a runtime change rather than a type-only one. With isolatedModules on, tsc stops inlining const enums: 33 emitted files change, consumers move from a baked-in literal to a property lookup, and lib/constants.js now emits ShouldMigrate, NativePlatformStatus, DebugTools, TrackActionNames, BuildStates and PlatformTypes as runtime objects.
Values are unchanged (verified at runtime), the suite is unchanged at 1513 passing, and the CLI still reports its version and renders the full help.
Arguably that convergence is the point: oxc and esbuild cannot inline a cross-file const enum, so today tsc's output disagrees with every other transpiler's for exactly those six enums. Now they agree.
It also makes the const modifier on the ten remaining declarations decorative, since it no longer buys inlining — converting them to plain enum is a sensible follow-up.
Happy to split this commit into its own PR stacked on this one if you would rather keep this change purely a refactor.