Skip to content

TypeScript Configuration Improvements - #1293

Closed
gugu8intel-i9 wants to merge 1 commit into
CodebuffAI:mainfrom
gugu8intel-i9:patch-1
Closed

TypeScript Configuration Improvements#1293
gugu8intel-i9 wants to merge 1 commit into
CodebuffAI:mainfrom
gugu8intel-i9:patch-1

Conversation

@gugu8intel-i9

Copy link
Copy Markdown

I improved it. I removed the browser-specific DOM library and switched module handling to NodeNext. idk what else to say, it's just a json file.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@codebuff-team

Copy link
Copy Markdown
Contributor

Thanks for the PR, but this touches a shared tsconfig.base.json that is almost certainly extended by many packages (including ones with browser code and ones with non-src/dist layouts), and the change set is a wholesale rewrite rather than a fix:

  • Removing DOM from lib will break any package that compiles browser-facing code and relies on this base config for its lib list.
  • Switching from module: esnext / moduleResolution: bundler to NodeNext/NodeNext is a major semantic change - it enforces Node's ESM resolution rules (explicit file extensions, no bare directory imports, etc.) across every consumer of this base config. That's a repo-wide migration, not a config tweak, and it directly conflicts with the allowImportingTsExtensions option you removed (which was there to support the bundler resolution mode).
  • Adding rootDir: "src", outDir: "dist", and include: ["src/**/*.ts"] to the base config assumes every package uses that exact layout. Any package that doesn't (or that overrides noEmit because it's bundler-only) will now get broken output paths or files silently excluded from compilation.
  • exactOptionalPropertyTypes and noUncheckedIndexedAccess are good strictness options in isolation, but flipping them on globally will produce a large wave of new type errors across the codebase with no corresponding fixes in this PR.

The PR description itself ('idk what else to say, it's just a json file') suggests this wasn't tested against the actual packages that consume this config. A change like this needs to be scoped per-package, tested against a full build, and justified with the specific problem it solves rather than applied as a blanket 'improvement'. As-is, this would break the build for consumers of this file, so I'm closing it.

@codebuff-team codebuff-team added bot:triaged Classified by the community triage bot pr:rejected Not a change this project wants; closed with an explanation labels Sep 7, 2026
@gugu8intel-i9

Copy link
Copy Markdown
Author

Thanks for the PR, but this touches a shared tsconfig.base.json that is almost certainly extended by many packages (including ones with browser code and ones with non-src/dist layouts), and the change set is a wholesale rewrite rather than a fix:

  • Removing DOM from lib will break any package that compiles browser-facing code and relies on this base config for its lib list.
  • Switching from module: esnext / moduleResolution: bundler to NodeNext/NodeNext is a major semantic change - it enforces Node's ESM resolution rules (explicit file extensions, no bare directory imports, etc.) across every consumer of this base config. That's a repo-wide migration, not a config tweak, and it directly conflicts with the allowImportingTsExtensions option you removed (which was there to support the bundler resolution mode).
  • Adding rootDir: "src", outDir: "dist", and include: ["src/**/*.ts"] to the base config assumes every package uses that exact layout. Any package that doesn't (or that overrides noEmit because it's bundler-only) will now get broken output paths or files silently excluded from compilation.
  • exactOptionalPropertyTypes and noUncheckedIndexedAccess are good strictness options in isolation, but flipping them on globally will produce a large wave of new type errors across the codebase with no corresponding fixes in this PR.

The PR description itself ('idk what else to say, it's just a json file') suggests this wasn't tested against the actual packages that consume this config. A change like this needs to be scoped per-package, tested against a full build, and justified with the specific problem it solves rather than applied as a blanket 'improvement'. As-is, this would break the build for consumers of this file, so I'm closing it.

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

Labels

bot:triaged Classified by the community triage bot pr:rejected Not a change this project wants; closed with an explanation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants