Fix TypeScript ESLint Phase 2a: Simple non-breaking fixes (27 errors) by justin808 · Pull Request #793 · shakacode/shakapacker

justin808 · 2025-11-02T19:51:24Z

Summary

This PR addresses simple, safe ESLint fixes that don't change any runtime behavior or APIs.

Part of #789 - TypeScript ESLint Phase 2a: Simple Non-Breaking Fixes
Related to #783 - TypeScript ESLint Technical Debt resolution
Follows PR #788 - Phase 1 completion

Changes Made

1. Remove Unused ESLint Directives (4 errors)

File: package/utils/debug.ts

Removed unused // eslint-disable-next-line no-console comments. These directives are no longer needed since the no-console rule is not triggering for these console methods in a debug utility module.

Impact: Cleanup only - no functional changes

2. Fix Explicit `any` Types (4 errors)

Files:

package/utils/getStyleRule.ts - Replaced any[] with unknown[] for loader arrays
package/utils/helpers.ts - Replaced error: any with error: unknown and added proper type assertion
package/utils/requireOrError.ts - Replaced any return type with unknown

Impact: Internal only - improves type safety without changing behavior

3. Fix Redundant Type Constituents (19 errors)

Files:

package/types.ts - Simplified DevServerConfig interface type unions
package/webpackDevServerConfig.ts - Simplified webpack dev server config type unions

Examples:

// Before:
allowed_hosts?: "all" | "auto" | string | string[]
host?: "local-ip" | "local-ipv4" | "local-ipv6" | string
port?: "auto" | string | number
static?: boolean | string | unknown
watch_files?: string | string[] | unknown

// After:
allowed_hosts?: string | string[]
host?: string
port?: string | number
static?: unknown
watch_files?: unknown

Reasoning:

When a union includes string, specific string literals like "all" or "auto" are redundant because string already includes all possible strings
When a union includes unknown, all other types are redundant because unknown is the top type that includes everything

Impact: Type definitions become cleaner and more accurate. No runtime changes, no API changes.

Error Reduction

Before: 247 ESLint errors
After: 220 ESLint errors
Fixed: 27 errors (10.9% reduction)

Testing

✅ All existing linting passes with standard ignore patterns
✅ TypeScript compilation succeeds (tsc --noEmit)
✅ Pre-commit hooks pass (type checking, linting, prettier)
✅ Security validation tests pass
⚠️ Some build tests fail, but these failures exist on main and are unrelated to these changes

Remaining Work

After this PR:

220 errors remaining (down from original 293)
Next: Phase 2b - Type Safety Improvements (~94 errors) - Issue TypeScript ESLint Phase 2b: Type Safety Improvements (~94 errors) #790

Related Issues and PRs

TypeScript ESLint Technical Debt: 381 Suppressed Errors Requiring Resolution #783 - Parent issue: TypeScript ESLint Technical Debt
TypeScript ESLint Phase 2a: Simple Non-Breaking Fixes (27 errors) #789 - This issue: Phase 2a Simple Fixes
Fix TypeScript ESLint errors (Phase 1): Auto-fixes and simple manual fixes #788 - Phase 1 completion (46 errors fixed)
TypeScript ESLint Phase 2b: Type Safety Improvements (~94 errors) #790 - Phase 2b: Type Safety Improvements (next step)

🤖 Generated with Claude Code

Summary by CodeRabbit

Refactor
- Refined type constraints and annotations across configuration and utility modules for improved type safety.
- Simplified error handling with more precise type definitions.
- Removed unnecessary linting directives from utility functions.

This PR addresses simple, safe ESLint fixes that don't change any runtime behavior or APIs. Part of #789 - TypeScript ESLint Phase 2a: Simple Non-Breaking Fixes Changes: 1. Remove unused ESLint directives (4 errors) - Removed unused `// eslint-disable-next-line no-console` comments in debug.ts 2. Fix explicit `any` types (4 errors) - Replaced `any` with `unknown` in getStyleRule.ts for loader arrays - Replaced `any` with `unknown` in helpers.ts error handling - Replaced `any` with `unknown` in requireOrError.ts return type 3. Fix redundant type constituents (19 errors) - Simplified union types in types.ts DevServerConfig interface - Simplified union types in webpackDevServerConfig.ts - Removed redundant specific strings when `string` type was present - Removed redundant types when `unknown` was present Examples: - `"all" | "auto" | string` → `string` (string already includes all strings) - `boolean | string | unknown` → `unknown` (unknown already includes all types) Error reduction: 247 → 220 errors (27 errors fixed) Impact: Internal only - no runtime behavior changes, no API changes Part of issue #783 - TypeScript ESLint Technical Debt resolution 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>

coderabbitai · 2025-11-02T19:51:32Z

Walkthrough

This PR refines TypeScript type safety across the package by replacing any types with unknown, simplifying union types in DevServerConfig and WebpackDevServerConfig interfaces, removing eslint-disable comments from debug utilities, and correcting error handling type annotations in utility functions.

Changes

Cohort / File(s)	Summary
Configuration type refinements `package/types.ts`, `package/webpackDevServerConfig.ts`	Narrowed `allowed_hosts` from `"all" \| "auto" \| string \| string[]` to `string \| string[]`; broadened `host` from `"local-ip" \| "local-ipv4" \| "local-ipv6" \| string` to `string`; narrowed `port` from `"auto" \| string \| number` to `string \| number`; simplified `static`, `watch_files`, `web_socket_server`, and `server` unions by removing enum-like literal constraints and `ServerType`/`WebSocketType` references
Type safety improvements `package/utils/getStyleRule.ts`, `package/utils/requireOrError.ts`, `package/utils/helpers.ts`	Changed `any[]` to `unknown[]` for `StyleRule.use` and `preprocessors` parameter; changed `requireOrError` return type from `any` to `unknown`; corrected error handling in `packageFullVersion` to type catch clause as `unknown` and access `.code` via `(error as NodeJS.ErrnoException).code`
Code cleanup `package/utils/debug.ts`	Removed eslint-disable-next-line no-console comments preceding console calls

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~15 minutes

Type narrowing/broadening decisions in types.ts and webpackDevServerConfig.ts should be verified to confirm they don't unintentionally restrict or broaden valid user configurations
Enum literal removals ("all", "auto", "local-ip" variants) may affect backwards compatibility depending on how users currently consume these types
Error handling cast in helpers.ts using NodeJS.ErrnoException should be checked for cross-platform compatibility

Possibly related issues

Addresses TypeScript typing debt and ESLint technical-debt issues (TypeScript ESLint Technical Debt: 381 Suppressed Errors Requiring Resolution #783, TypeScript ESLint Phase 2a: Simple Non-Breaking Fixes (27 errors) #789, TypeScript ESLint Phase 2b: Type Safety Improvements (~94 errors) #790) by replacing any with unknown and removing redundant type literals across configuration and utility files

Possibly related PRs

Related to earlier dev-server type declaration modifications (Convert NPM package to TypeScript (Issue #200) #602, Improve types #555) that introduced or adjusted the exact DevServerConfig and WebpackDevServerConfig fields being refined here

Suggested reviewers

tomdracz
G-Rath

Poem

🐰 Types now clearer, any no more,
unknown whispers from the drawer,
No console lint-noise to ignore,
Dev server blessed with bounds galore!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The PR title "Fix TypeScript ESLint Phase 2a: Simple non-breaking fixes (27 errors)" is directly related to the changeset and clearly summarizes the main changes. The title accurately identifies that the PR contains TypeScript and ESLint fixes organized as part of Phase 2a, and the number of errors fixed (27) matches the PR objectives. The changes across all files—removing unused ESLint directives, replacing `any` types with `unknown`, and simplifying redundant type unions—align precisely with what the title conveys. The title is specific, clear, and concise without unnecessary noise, making it easy for a reviewer scanning the history to understand the primary purpose of the changes.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch justin808/fix-eslint-phase-2a

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

package/utils/helpers.ts (1)
66-77: Consider using the isModuleNotFoundError helper for consistency.

The error handling at line 69 directly checks (error as NodeJS.ErrnoException).code without verifying the property exists, while lines 12-17 use the isModuleNotFoundError helper. This inconsistency could lead to runtime errors if the caught error doesn't have a code property.

Apply this diff to use the helper consistently:
  } catch (error: unknown) {
    // Re-throw the error with proper code to maintain compatibility with babel preset
    // The preset expects MODULE_NOT_FOUND errors to handle missing core-js gracefully
-    if ((error as NodeJS.ErrnoException).code === "MODULE_NOT_FOUND") {
+    if (isModuleNotFoundError(error)) {
      throw error
    }

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f81f6e5 and fd42a95.

📒 Files selected for processing (6)

package/types.ts (2 hunks)
package/utils/debug.ts (0 hunks)
package/utils/getStyleRule.ts (1 hunks)
package/utils/helpers.ts (1 hunks)
package/utils/requireOrError.ts (1 hunks)
package/webpackDevServerConfig.ts (2 hunks)

💤 Files with no reviewable changes (1)

package/utils/debug.ts

🔇 Additional comments (7)

package/utils/getStyleRule.ts (1)

9-9: LGTM: Type safety improvement from any[] to unknown[].

The change from any[] to unknown[] appropriately tightens type safety for webpack loader arrays without affecting runtime behavior.

Also applies to: 15-15

package/utils/requireOrError.ts (1)

9-9: LGTM: Return type correctly changed to unknown.

The return type change from any to unknown is appropriate since require() returns module contents of unknown type at compile time. Callers will need to assert or narrow the type as needed.

package/types.ts (3)

66-66: Verify that removing string literal unions doesn't impact developer experience.

The removal of specific string literals ("all", "auto", "local-ip", etc.) from union types is technically correct since they're subsumed by the string type. However, these literals may have provided IDE autocomplete and validation benefits for developers.

Please confirm that these specific values are documented elsewhere or that the loss of type-level autocomplete is acceptable for the project.

Also applies to: 74-74, 88-88

91-92: LGTM: Correctly simplified redundant unions with unknown.

Lines 91-92 correctly simplify the unions since unknown is the top type that subsumes all other types (e.g., boolean | string | unknown → unknown).

97-97: Verified: Removed type references are safely localized.

The search confirms that WebSocketType and ServerType are only defined at lines 56–57 in package/types.ts with no other references in the codebase. The type removals at lines 97 and 103 do not impact any external code.

package/webpackDevServerConfig.ts (2)

28-28: Type simplifications align with package/types.ts changes.

The removal of string literals from allowedHosts, host, and port mirrors the changes in DevServerConfig (package/types.ts). While technically correct, consider whether the loss of IDE autocomplete for values like "local-ip" or "auto" impacts developer experience.

Also applies to: 32-32, 45-45

50-50: LGTM: Correctly simplified watchFiles to unknown.

The simplification from string | string[] | unknown to unknown is correct since unknown subsumes the other types in the union.

claude · 2025-11-02T19:55:39Z

Code Review: PR #793 - TypeScript ESLint Phase 2a

Overall Assessment

✅ Approved with minor suggestions

This is a well-structured, focused PR that makes safe type safety improvements without changing runtime behavior. The changes are conservative, well-documented, and align with the project's ESLint cleanup roadmap.

Code Quality: Excellent

Strengths

Type Safety Improvements
- Replacing any with unknown in package/utils/getStyleRule.ts:9,15, package/utils/helpers.ts:66, and package/utils/requireOrError.ts:9 is the correct approach. This forces proper type checking at usage sites while maintaining flexibility.
- The type assertion pattern in helpers.ts:69 is appropriate: (error as NodeJS.ErrnoException).code safely narrows the type when needed.
Clean Redundant Type Unions
- Excellent simplification of redundant union types:
  - "all" | "auto" | string → string (correct: string literals are redundant when string is present)
  - boolean | string | unknown → unknown (correct: unknown is the top type)
- This improves clarity and makes the type system more maintainable.
ESLint Directive Cleanup
- Removing unused // eslint-disable-next-line no-console comments in package/utils/debug.ts is appropriate since console methods are intentional in debug utilities.

Potential Issues: None Critical

Minor Observations

Type Narrowing Pattern (package/utils/helpers.ts:69)
- Current: (error as NodeJS.ErrnoException).code
- This is safe and appropriate. The optional chaining approach would be more defensive: (error as NodeJS.ErrnoException)?.code, but not necessary here since we're in a catch block.
- Impact: Low - Current approach is acceptable.
DevServerConfig Type Simplification (package/types.ts:66-103)
- Removing string literals like "local-ip" from host?: string means documentation of valid values is now only in comments.
- Consider whether TypeScript users would benefit from the literal types for auto-completion.
- Impact: Low - The change follows the principle that runtime accepts any string, and webpack docs remain authoritative.
WebSocketType and ServerType Removal (package/types.ts:94-102)
- Removing type references for WebSocketType and ServerType from unions with string is logically correct (string already includes them).
- Good catch on this redundancy!

Performance: No Impact

All changes are compile-time only. Runtime performance is unchanged.

Security: No Concerns

Error handling in helpers.ts properly preserves error context
Type narrowing doesn't introduce any unsafe operations
No sensitive data handling affected

Test Coverage: Adequate

Existing tests at test/package/helpers.test.js and test/typescript/requireOrError.test.js should continue to pass
The changes don't require new tests since they're internal type improvements without behavior changes
✅ PR description confirms TypeScript compilation and pre-commit hooks pass

Best Practices: Followed

✅ Changes align with CLAUDE.md guidelines:

Focused, minimal changes
No unnecessary comments
Follows existing code conventions
Part of a well-planned incremental improvement strategy

Suggestions

Optional Improvements

Documentation Update (Optional)
- Consider adding JSDoc comments to explain why unknown is used instead of any in getStyleRule and requireOrError for future maintainers.
- Example:
```
/**
 * @param preprocessors Array of webpack loaders (typed as unknown for flexibility)
 */
```
Changelog Entry (Not Required)
- Per CLAUDE.md: These are internal refactoring changes, so no changelog entry is needed ✅

Final Verdict

Status: ✅ Ready to Merge

This PR successfully:

Reduces ESLint errors by 27 (10.9% reduction)
Improves type safety without breaking changes
Maintains backward compatibility
Follows project conventions
Is well-documented and tested

The incremental approach to ESLint cleanup is commendable. Phase 2a tackles the low-hanging fruit effectively, setting up for more complex type safety improvements in Phase 2b.

Reviewed by Claude Code

## Summary This PR addresses #790 by enabling TypeScript linting for all package files and fixing the primary type safety issues related to config objects. This is **Phase 2b** of the TypeScript ESLint technical debt resolution. ## Key Changes ### 1. Enabled TypeScript Linting - Removed `package/**/*.ts` from ESLint ignore patterns - All TypeScript files in `package/` are now linted (except as specifically overridden) ### 2. Deferred CommonJS Migration to Phase 3 - Added global suppression for `@typescript-eslint/no-require-imports` - This allows us to fix type safety issues while deferring the CommonJS-to-ESM migration to a future breaking change release ### 3. Fixed Config Type Safety (Primary Goal) Added proper `Config` type assertions in 11 files that `require("../config")`: - `package/environments/base.ts` - `package/environments/development.ts` - `package/environments/production.ts` - `package/environments/test.ts` - `package/plugins/webpack.ts` - `package/plugins/rspack.ts` - `package/dev_server.ts` - `package/rspack/index.ts` - `package/rules/raw.ts` - `package/utils/getStyleRule.ts` - `package/utils/requireOrError.ts` ### 4. Cleanup - Removed unused `eslint-disable` directives for `global-require` - Removed unused type definitions (`ServerType`, `WebSocketType`) ### 5. Updated ESLint Config Overrides Updated override blocks to suppress remaining type safety errors in: - Utility files that use dynamic requires - Plugin/optimization files that access webpack/rspack modules dynamically - Files that will require more extensive refactoring ## Impact ### Errors Fixed - **Before**: 112 ESLint errors when TypeScript files were linted - **After**: 0 ESLint errors ✅ - **Fixed**: 65+ type safety errors related to config object access ### Type Safety Improvements - Config objects now have proper typing throughout the codebase - TypeScript can now catch config-related type errors at compile time - Improved IDE autocomplete and documentation for config properties ### Remaining Work The remaining ~47 type safety errors in utility files, plugin loaders, and dynamic module requires are now properly documented in `eslint.config.js` with override blocks. These will be addressed in future PRs as they require: - Proper typing for dynamic webpack/rspack plugin requires - Refactoring of utility functions to use proper types instead of `any` - Potentially creating type definitions for third-party modules ## Testing - ✅ `yarn lint` passes - ✅ `bundle exec rubocop` passes - ✅ Type checking passes - ✅ Pre-commit hooks pass - ⚠️ Test suite has same pass/fail rate as main (11 failing suites are pre-existing) ## Related Issues - Fixes #790 - TypeScript ESLint Phase 2b: Type Safety Improvements - Follows #789 (PR #793) - Phase 2a simple fixes - Part of #783 - TypeScript ESLint Technical Debt resolution 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude <[email protected]>

This was referenced Nov 2, 2025

TypeScript ESLint Phase 2a: Simple Non-Breaking Fixes (27 errors) #789

Closed

TypeScript ESLint Technical Debt: 381 Suppressed Errors Requiring Resolution #783

Closed

coderabbitai Bot reviewed Nov 2, 2025

View reviewed changes

coderabbitai Bot approved these changes Nov 2, 2025

View reviewed changes

justin808 merged commit f8664b1 into main Nov 2, 2025
55 checks passed

justin808 deleted the justin808/fix-eslint-phase-2a branch November 2, 2025 21:13

justin808 mentioned this pull request Nov 2, 2025

Enable TypeScript linting and fix config type safety (#790) #796

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix TypeScript ESLint Phase 2a: Simple non-breaking fixes (27 errors)#793

Fix TypeScript ESLint Phase 2a: Simple non-breaking fixes (27 errors)#793
justin808 merged 1 commit intomainfrom
justin808/fix-eslint-phase-2a

justin808 commented Nov 2, 2025 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Nov 2, 2025 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

claude Bot commented Nov 2, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

justin808 commented Nov 2, 2025 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes Made

1. Remove Unused ESLint Directives (4 errors)

2. Fix Explicit any Types (4 errors)

3. Fix Redundant Type Constituents (19 errors)

Error Reduction

Testing

Remaining Work

Related Issues and PRs

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Nov 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related issues

Possibly related PRs

Suggested reviewers

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

claude Bot commented Nov 2, 2025

Code Review: PR #793 - TypeScript ESLint Phase 2a

Overall Assessment

Code Quality: Excellent

Strengths

Potential Issues: None Critical

Minor Observations

Performance: No Impact

Security: No Concerns

Test Coverage: Adequate

Best Practices: Followed

Suggestions

Optional Improvements

Final Verdict

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

justin808 commented Nov 2, 2025 •

edited by coderabbitai Bot

Loading

2. Fix Explicit `any` Types (4 errors)

coderabbitai Bot commented Nov 2, 2025 •

edited

Loading