Switch to moduleResolution: bundler#305
Conversation
tsgo requires moduleResolution: "bundler" which strictly follows package.json exports fields. Fix @tko/* resolution by adding explicit paths mapping to source files in tsconfig. Changes: - Add @typescript/native-preview (tsgo) as devDependency - moduleResolution: node10 → bundler - Add @tko/* paths for source resolution during development - Export Subscription type from @tko/observable - Fix deep import in bindingEvent.ts to use package root - Add type annotation to binding.core bindings const Both tsc and tsgo pass with 0 errors. Speed: tsc 1.0s → tsgo 0.17s (6x faster). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR updates TypeScript configuration with new module resolution settings and import path aliases, consolidates type imports across packages, adds explicit type annotations to exported bindings, exports the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Pull request overview
This PR introduces the TypeScript 7 Go-native typechecker (tsgo) alongside existing tsc usage, and updates TypeScript module resolution to "bundler" (required by tsgo) with accompanying source-resolution fixes across @tko/* workspace packages.
Changes:
- Add
@typescript/native-preview(tsgo) to devDependencies and lockfile. - Switch
tsconfig.jsontomoduleResolution: "bundler"and add@tko/*path mappings to resolve to workspace source entrypoints. - Remove a deep import by exporting
Subscriptionfrom@tko/observableand updating consumers; add a workaround type annotation forbindingsin@tko/binding.core.
Reviewed changes
Copilot reviewed 4 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.json | Switches to bundler resolution and adds @tko/* path mappings for workspace source resolution. |
| package.json | Adds the @typescript/native-preview dependency needed to run tsgo. |
| bun.lock | Locks @typescript/native-preview and its platform-specific optional dependencies. |
| packages/observable/src/index.ts | Exposes Subscription type at the package root to avoid deep imports. |
| packages/bind/src/bindingEvent.ts | Updates Subscription import to use the package root export under bundler resolution. |
| packages/binding.core/src/index.ts | Adds an explicit type to bindings to avoid resolution/inference issues under bundler mode. |
| "@eslint/js": "^10.0.1", | ||
| "@types/jquery": "^4.0.0", | ||
| "@types/mocha": "^10.0.10", | ||
| "@typescript/native-preview": "^7.0.0-dev.20260414.1", |
There was a problem hiding this comment.
@typescript/native-preview is referenced with a caret range (^7.0.0-dev.20260414.1). Because this is a dev snapshot, allowing semver-compatible upgrades can unexpectedly pull in a different preview build (or eventually stable 7.0.0) and change typechecking behavior across environments. Consider pinning to an exact version (no ^) or otherwise constraining updates so CI/local installs remain reproducible.
| "@typescript/native-preview": "^7.0.0-dev.20260414.1", | |
| "@typescript/native-preview": "7.0.0-dev.20260414.1", |
| import { using } from './using' | ||
|
|
||
| export const bindings = { | ||
| export const bindings: Record<string, unknown> = { |
There was a problem hiding this comment.
Typing bindings as Record<string, unknown> forces all handler values to unknown, which discards useful type information for downstream consumers (e.g., no IntelliSense/compile-time checking when accessing specific bindings). If the goal is only to ensure the object conforms to a shape for module-resolution/inference reasons, consider using a satisfies Record<string, unknown> constraint (or a more specific binding-handler type) so the object keeps its inferred property types while still meeting the required contract.
|
Tsgo is still in preview. Don't merge it. |
The "*" wildcard paths were a pre-bundler workaround. Now that "@tko/*" has its own explicit mapping, the wildcard is unused and adds unnecessary resolution complexity. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Target/Module: ES2022 (was ES6/ES2015) - moduleResolution: bundler - Add bunx tsgo to build commands - Update path alias description Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
README.md: - npm→bun, electron→Vitest/Playwright throughout - Remove stale test-coverage target, SauceLabs, standard references - Simplify WSL setup to just playwright install AGENTS.md: - Fix package count (25→26) - npx changeset→bunx changeset plans/modern-tooling.md: - Phase 1 merged, Phase 2 PR open Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Pin @typescript/native-preview to exact version (no ^) - Add bunfig.toml with minimumReleaseAge = 172800 (48h) - Keep Record<string, unknown> annotation on bindings (tsgo needs it) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@phillipc could you please elaborate? That's a strong opinion, is there a specific risk you're concerned about? |
|
@phillipc — if you have concerns about this PR, happy to discuss. Here's a summary of the risk assessment: What this PR actually changes:
What it doesn't change:
Size: 10 files, +62/-59 lines. Mostly config and docs. The |
|
a) moduleResolution: "bundler" - It's fine, and I even tried it myself. This has nothing to do with tsgo alone. b) Using unfinished tools creates technical debt and makes long-term stability harder to manage. Breaking-changes, Bugs and co. are daily business.. c) You said it yourself, we're not really using the new dependency, too. |
Per phillipc's feedback: tsgo is a preview tool not used in CI. Keep moduleResolution: bundler (which he confirmed is fine) but remove the tsgo devDependency. Developers can install it locally if they want faster typechecking. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@phillipc Good points — removed The PR now just does the |
Summary
Add TypeScript 7's Go-native compiler (
tsgo) alongside tsc. Switch tomoduleResolution: "bundler"which is required by tsgo and stricter about package resolution.Speed
6x faster. Both produce 0 errors.
What changed (6 files, +28/-9)
package.json— add@typescript/native-previewtsconfig.json—moduleResolution: "bundler", add@tko/*paths for source resolution, remove deprecatedignoreDeprecationspackages/observable/src/index.ts— exportSubscriptiontype (was only available via deep import)packages/bind/src/bindingEvent.ts— importSubscriptionfrom package root instead of deep pathpackages/binding.core/src/index.ts— add type annotation tobindingsconst (bundler resolution inference issue)Why these source changes?
moduleResolution: "bundler"strictly followspackage.jsonexportsfields. TKO's@tko/*workspace packages haveexportspointing to./dist/index.js, which has no type information. The@tko/*paths mapping in tsconfig tells TypeScript to resolve to source (index.ts) during development.Test plan
bunx tsgo— 0 errorsbunx tsc— 0 errorsmake— 27 packages buildbunx vitest run— 143 files, 2679 passed🤖 Generated with Claude Code
Summary by CodeRabbit
Chores
Refactor