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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature request: Consider TS imports with all type the same as type-only. #873

Closed
alvaro-cuesta opened this issue Nov 23, 2023 · 4 comments · Fixed by #875
Closed

Feature request: Consider TS imports with all type the same as type-only. #873

alvaro-cuesta opened this issue Nov 23, 2023 · 4 comments · Fixed by #875

Comments

@alvaro-cuesta
Copy link
Contributor

alvaro-cuesta commented Nov 23, 2023

Context

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

This can be fixed by moving the individual type imports to a separate type import, 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.

Some of us prefer this syntax since it makes it easier to mix type and non-type imports.

Expected Behavior

This rule:

{
  name: 'not-to-dev-dep',
  severity: 'error',
  comment:
    "This module depends on an npm package from the 'devDependencies' section of your " +
    'package.json. It looks like something that ships to production, though. To prevent problems ' +
    "with npm packages that aren't there on production declare it (only!) in the 'dependencies'" +
    'section of your package.json. If this module is development only - add it to the ' +
    'from.pathNot re of the not-to-dev-dep rule in the dependency-cruiser configuration',
  from: {
    path: '^(app)',
    pathNot: [
      '.(spec|test).(js|mjs|cjs|ts|ls|coffee|litcoffee|coffee.md)$',
      // allow dev depencendies in modules/testing as those are helpers for tests
      'modules/testing',
    ],
  },
  to: {
    dependencyTypes: ['npm-dev'],
    dependencyTypesNot: ['type-only'],
  },
},

(Note the dependencyTypesNot: ['type-only'] line which is new, different from default config.)

Should not be fired when using imports like this:

import { type SomeType, type OtherType } from 'my-dev-dependency';

Current Behavior

Correctly ignores devDependencies imports when using this syntax:

import type { SomeType, OtherType } from 'my-dev-dependency';

But this syntax trips the rule:

import { type SomeType, type OtherType } from 'my-dev-dependency';

Possible Solution

All members inside the import statement are qualified as type so it could be considered as 'type-only' for rule matching purposes.

Additionally I propose adding dependencyTypesNot: ['type-only'] to the default generated not-to-dev-dep rule. Importing types from dev dependencies is safe since they are erased at TS compile time. This would be a good way to ensure you're only importing types from dev dependencies.

Considered alternatives

  1. Possibly leave it as is and just force using import type { ... } explicitly.
    • See "Context" section above on why it is inconvenient.
  2. Introduce a new dependency type ('all-types'?)
    • Seems like it doesn't add much value and complicates things further.
@alvaro-cuesta
Copy link
Contributor Author

I'd be willing to work on this if the suggested change is deemed appropriate.

@sverweij
Copy link
Owner

hi @alvaro-cuesta yep - good one to add! I missed that one when adding type only imports 😬.

Agree with slotting them in with the imports detected as type-only - and a pull request to add them is welcome.

The addition to the not-to-dev-dep rule in the default rule set is a good one as well. It should've been part of it when type only imports were introduced in dependency-cruiser. I've added it in #874.

sverweij added a commit that referenced this issue Nov 24, 2023
…es (#874)

## Description

- adds an exception to the `not-to-dev-dep` for type-only dependencies
to ones in `@types` folders (which are _pretty_ likely to be type only
as well, even when not explicitly imported as such)
- does the same for dependency-cruiser's own configuration

## Motivation and Context

Implements the second suggestion from #873 

## How Has This Been Tested?

- [x] green ci
- [x] manually against a TS repo

## 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)
- [x] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing
functionality to change)
sverweij added a commit that referenced this issue Nov 26, 2023
…KING (#875)

## 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](https://www.typescriptlang.org/docs/handbook/release-notes/typescript-4-5.html#type-modifiers-on-import-names).
`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'`](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/docs/rules/consistent-type-imports.md#fixstyle)
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?

- [x] green ci
- [x] 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)
- [x] Breaking change (fix or feature that would cause existing
functionality to change)

## Checklist

- [x] 📖

  - My change doesn't require a documentation update, or ...
  - it _does_ and I have updated it

- [x] ⚖️
- The contribution will be subject to [The MIT
license](https://github.com/sverweij/dependency-cruiser/blob/main/LICENSE),
and I'm OK with that.
  - The contribution is my own original work.
- I am ok with the stuff in
[**CONTRIBUTING.md**](https://github.com/sverweij/dependency-cruiser/blob/main/.github/CONTRIBUTING.md).

---------

Co-authored-by: Sander Verweij <sverweij@users.noreply.github.com>
@sverweij
Copy link
Owner

Hi @alvaro-cuesta as you can see the PR is merged - thanks again!

As it's a breaking change I'd like to combine its release with the other still open PR, so it's not part of today's release. Realising it might be useful nonetheless (and folks might want to try it out & provide feedback) I released both your PR and the other one with the beta tag as dependency-cruiser@16.0.0-beta-1 on npmjs.

@alvaro-cuesta
Copy link
Contributor Author

@sverweij my pleasure. Thanks for also releasing the beta version, I just bumped it in my project ❤️

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 a pull request may close this issue.

2 participants