Skip to content

Conversation

@som-sm
Copy link
Collaborator

@som-sm som-sm commented Oct 11, 2025

This PR ensures import-path rule works on re-exports as well. This is required because even re-export sources could get autocompleted with .js extension.

Screen.Recording.2025-10-12.at.12.03.32.AM.mov

In the video above, the re-export file extension changes from .d.ts to .js if "update imports" suggestion is applied.

@som-sm som-sm requested a review from sindresorhus October 11, 2025 18:39
@claude
Copy link

claude bot commented Oct 11, 2025

Code Review

Thank you for this contribution! This PR addresses an important issue where IDE autocomplete can incorrectly suggest .js extensions for re-exports in TypeScript declaration files.

✅ Positives

  1. Clear problem statement: The video demonstration clearly shows the issue being fixed.
  2. Minimal, focused changes: The PR modifies only what's necessary to solve the problem.
  3. Correct implementation: The logic properly handles all three export types that can have a source property:
    • ImportDeclaration (existing)
    • ExportNamedDeclaration (new - handles export { foo } from '...')
    • ExportAllDeclaration (new - handles export * from '...')
  4. Proper guard clause: The if (!node.source) check correctly filters out regular exports that don't have an import source.
  5. Config update: Adding index.d.ts to the file pattern in xo.config.js:65 ensures the rule now covers the main re-export file.

📝 Observations

Code Quality: The implementation is sound and follows ESLint plugin patterns correctly.

Edge Cases Covered:

  • ✅ Regular exports without source (e.g., export { foo }) - correctly skipped via !node.source check
  • ✅ Re-exports with source (e.g., export * from './foo') - correctly handled
  • ✅ Named re-exports (e.g., export { Foo } from './foo') - correctly handled

Testing: This PR modifies a lint rule but does not include tests. Looking at the repo structure, I notice that import-path.js does not have a corresponding test file, while require-export.js and require-exported-types.js do have test files. While not blocking, adding a test file like lint-rules/import-path.test.js would help ensure this rule continues to work correctly as the codebase evolves.

🔍 Suggested Improvements (Optional)

Consider adding tests (can be a follow-up PR):

  • Test ImportDeclaration with .d.ts (should pass)
  • Test ImportDeclaration with .js (should fail)
  • Test ExportNamedDeclaration with .d.ts (should pass)
  • Test ExportNamedDeclaration with .js (should fail)
  • Test ExportAllDeclaration with .d.ts (should pass)
  • Test ExportAllDeclaration with .js (should fail)
  • Test regular exports without source (should be ignored)

Error message update: The error message says "Import path" but now also applies to export paths. Consider updating the message in import-path.js to be more inclusive (e.g., "Import/export path").

✅ Recommendation

This PR is ready to merge as-is. The core functionality is correct and solves the stated problem. The suggested improvements above are nice-to-haves that could be addressed in follow-up work if desired.

The changes correctly:

  • Extend the lint rule to all relevant AST node types
  • Maintain backward compatibility
  • Follow existing code patterns
  • Include proper null-checking

Great work identifying and fixing this issue! 🎉

@sindresorhus sindresorhus merged commit 7698140 into main Oct 12, 2025
7 checks passed
@sindresorhus sindresorhus deleted the fix/make-import-path-rule-work-on-re-exports branch October 12, 2025 03:13
@som-sm som-sm mentioned this pull request Oct 16, 2025
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.

3 participants