Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(extract): detect 'type-only' with inline imports/exports #875

Merged

Conversation

alvaro-cuesta
Copy link
Contributor

@alvaro-cuesta alvaro-cuesta commented Nov 24, 2023

Description

Detect dependencies extracted from TypeScript as 'type-only' also when all of the inline imports/re-exports are qualified as type.

Motivation and Context

TypeScript 4.5 added inline type imports. dependency-cruiser has support for 'type-only' in dependencyTypesNot, but this only works when using type import statements, and not when regular import statements have all their inline imports qualified as type.

This can be fixed by moving the individual type imports to a separate type import statement, but this is cumbersome for some of us since using ESLint's consistent-type-imports with fixStyle: 'inline-type-imports' will prefer adding type inline even when all imports are type.

By extension this PR also implements the functionality for inline type re-exports.

Closes #873.

How Has This Been Tested?

  • green ci
  • additional unit tests

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • Documentation only change
  • Refactor (non-breaking change which fixes an issue without changing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • 馃摉

    • My change doesn't require a documentation update, or ...
    • it does and I have updated it
  • 鈿栵笍

    • The contribution will be subject to The MIT license, and I'm OK with that.
    • The contribution is my own original work.
    • I am ok with the stuff in CONTRIBUTING.md.

@alvaro-cuesta
Copy link
Contributor Author

alvaro-cuesta commented Nov 24, 2023

Unsure if this should be considered a new feature or a fix, a bit of a middle ground (opted for feature because it feels like adding functionality to detect more combinations).

Also unsure if this should be considered a breaking change (opted for "yes" but I'd appreciate your perspective): I think it's possible that there's a rule out there that might start rejecting with this change, right? My thought process was: dependencyTypesNot is less problematic since it will just start accepting more stuff, but if someone used 'type-only' in a rule using dependencyTypes, it might have been ignoring false negatives that the rule will start catching after this change.

(I wonder why eslint didn't pick up on this as there's a rule for that 馃し)
Copy link

codeclimate bot commented Nov 24, 2023

Code Climate has analyzed commit ef18b2e and detected 0 issues on this pull request.

View more on Code Climate.

Copy link
Owner

@sverweij sverweij left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alvaro-cuesta that was very quick! And a great PR

  • good documentation,
  • thorough unit tests,
  • comments where it adds value to future maintainers and
  • a refactor of the isTypeOnly function that makes it more readable

Thanks!

@sverweij
Copy link
Owner

Unsure if this should be considered a new feature or a fix, a bit of a middle ground (opted for feature because it feels like adding functionality to detect more combinations).

Agree.

Also unsure if this should be considered a breaking change (opted for "yes" but I'd appreciate your perspective): I think it's possible that there's a rule out there that might start rejecting with this change, right? My thought process was: dependencyTypesNot is less problematic since it will just start accepting more stuff, but if someone used 'type-only' in a rule using dependencyTypes, it might have been ignoring false negatives that the rule will start catching after this change.

Yep - it's the bane of maintaining a code analysis/ linting packages that improvements sometimes are breaking changes, even though the number of affected consumers might be close to zero. The other open PR (#867) has the same issue.

@sverweij sverweij merged commit 19033e8 into sverweij:main Nov 26, 2023
8 checks passed
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.

Feature request: Consider TS imports with all type the same as type-only.
2 participants