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

Madge for typescript: skipping type imports still produces wrong result while searching for circular dependencies #232

Open
vicrac opened this issue Dec 2, 2019 · 23 comments
Labels
core dependency tree generation

Comments

@vicrac
Copy link

vicrac commented Dec 2, 2019

I have
moduleA.ts

import * as Life from './moduleB';

export interface Person {
    age: number;
    health: number;
    // ...
}

export const live = (p: Person) => Life.goToDoctor(Life.getOld(p))

console.log(live({
    age: 65,
    health: 0.6,
}))

and moduleB.ts:

import { Person } from './moduleA'

export const getOld = (p: Person): Person => ({
    ...p,
    age: p.age + 1
})

export const goToDoctor = (p: Person): Person => ({
    ...p,
    health: Math.min(p.health * 1.2, 1)
})

Following advice from @pahen (see (#231)) I enabled skipping type imports in typescript (since cyclic deps are allowed in this case), so I added to my .madgerc file in root directory:

{
	"detectiveOptions": {
		"ts": {
			"skipTypeImports": true
		}
	}
}

but I'm still getting error:

Processed 2 files (584ms) 

✖ Found 1 circular dependency!

1) moduleA.ts > moduleB.ts

Is there something I am missing, or is it a bug?

@zekth
Copy link
Contributor

zekth commented Feb 25, 2020

Same for import type syntax:

foo.ts

import type { Bar } from 'bar'

export type Foo = {
  bar: string;
};

export function doFoo() {
  return true;
}

bar.ts

import type { Foo } from 'foo'

export type Bar = {
  bar: string;
};

export function doBar() {
  return true;
}

madge config:

{
    "detectiveOptions": {
      "ts": {
        "skipTypeImports": true
      }
    },
    "baseDir": "./src",
    "fileExtensions": [
      "ts"
    ],
    "excludeRegExp": [
      "^.*__tests__.*$",
      "^.*__mocks__.*$"
    ],
    "tsConfig": "tsconfig.json"
  }

Output:


✖ Found 1 circular dependency!

1) bar.ts > foo.ts

@emortlock
Copy link

There's a PR here dependents/detective-typescript#27 by @eelco which looks to add support for import type

@pahen
Copy link
Owner

pahen commented May 7, 2020

Could you see if Madge v3.9.0 works for you now @vicrac ?

@vicrac
Copy link
Author

vicrac commented May 18, 2020

@pahen unfortunately, I'm still experiencing the same issue.

@iamakulov
Copy link

unfortunately, I'm still experiencing the same issue.

I just had the same issue and figured out the reason.

Reason

dependents/detective-typescript#27 was released in detective-typescript@3.8. However, madge doesn’t use the detective-typescript package directly – instead, it calls it through dependency-treeprecinct packages.

detective-typescript@3.8 was only released recently, so if you already had dependency-tree or precinct installed, you like have an old version of it.

Upgrading Madge to v3.9 would install detective-typescript@3.8, since it includes it in node_modules – but only next to madge, not inside dependency-treeprecinct.

So this means that, if you already have dependency-tree or precinct in your node_modules, installing the latest Madge won’t make them use detective-typescript@3.8. And import type won’t become supported.

How to solve

There’s probably no library-level solution – unless you convince precinct set "detective-typescript": "^3.8" in their package.json and upgrade Madge to use that version of precinct.

Locally, I solved this by

  • removing all modules that depended on precinct (e.g., in my case, it was serverless and madge),
  • checking that precinct is not installed anymore,
  • and installed these modules again

This pulled the latest version of precinct dependencies, including detective-typescript@3.8.

Yarn resolutions can also help solving this.

@pahen
Copy link
Owner

pahen commented Jun 16, 2020

unfortunately, I'm still experiencing the same issue.

I just had the same issue and figured out the reason.

Reason

pahen/detective-typescript#27 was released in detective-typescript@3.8. However, madge doesn’t use the detective-typescript package directly – instead, it calls it through dependency-treeprecinct packages.

detective-typescript@3.8 was only released recently, so if you already had dependency-tree or precinct installed, you like have an old version of it.

Upgrading Madge to v3.9 would install detective-typescript@3.8, since it includes it in node_modules – but only next to madge, not inside dependency-treeprecinct.

So this means that, if you already have dependency-tree or precinct in your node_modules, installing the latest Madge won’t make them use detective-typescript@3.8. And import type won’t become supported.

How to solve

There’s probably no library-level solution – unless you convince precinct set "detective-typescript": "^3.8" in their package.json and upgrade Madge to use that version of precinct.

Locally, I solved this by

  • removing all modules that depended on precinct (e.g., in my case, it was serverless and madge),
  • checking that precinct is not installed anymore,
  • and installed these modules again

This pulled the latest version of precinct dependencies, including detective-typescript@3.8.

Yarn resolutions can also help solving this.

Thanks for looking into this. I released Madge v3.9.2 now which will hopefully work better since precinct is now updated to use all latest versions of the detectives including detective-typescript@3.8.

@pahen
Copy link
Owner

pahen commented Jun 16, 2020

Wanna give it a try again @vicrac ?

@Desnoo
Copy link

Desnoo commented Dec 16, 2020

@pahen I have similar issues with typescript > 4.0.

I have installed madge with version 3.12.0.
My Config is:

{
    "tsConfig": `${projectPath}/tsconfig.json`,
    "fileExtensions": ["ts"],
    "detectiveOptions": {
        "ts": {
             "skipTypeImports": true
        }
    }
}

@jonlepage
Copy link

jonlepage commented Dec 22, 2020

i can also confirm something wrong ! "madge": "^3.12.0",
using skipTypeImports not work
image

Actually the test look like thats
index.jsx

import { Test } from './1.tsx';

1.tsx

import { Dictionary } from './2.tsx';
export class Test implements Dictionary {
}

2.tsx

export interface Dictionary {}

2.tsx should not considerate because import only type.

@ArnaudBarre
Copy link

Working with madge 3.12 and this in package.json:

  "madge": {
    "detectiveOptions": {
      "tsx": {
        "skipTypeImports": true
      }
    }
  }

Dexterp37 added a commit to Dexterp37/glean.js that referenced this issue Apr 12, 2021
Importing as a type is available since TS 3.8 and allows
to import types that are exclusively used for annotations
and removed after the build.
This additionally changes the Madge configuration to not
complain when type imports are used. See pahen/madge#232
@matthias-ccri
Copy link

I just spent some time debugging this issue, just to find that I had mistyped .madgerc as .magderc. I feel really dumb but hopefully this will help someone.

@StanFlint
Copy link

StanFlint commented Nov 29, 2021

I am still encountering this issue with madge v5.0.1. Madge behaves as if the configuration skipTypeImports: true doesn't exist with typescript v4.4.2. Any advice would be greatly appreciated.

@StampixSMO
Copy link

This also doesn't work if you pass it by code:

const madge = require('madge');

madge('app/models/index.ts', {
  detectiveOptions: {
    es6: { skipTypeImports: true }
  }
}).then((res) => {
  console.log(res.circular());
});

Still returns circular type imports

@matthias-ccri
Copy link

Back again... this time I needed to add a "tsx" block to the .madgerc:

{
  "detectiveOptions": {
    "ts": {
      "skipTypeImports": true
    },
    "tsx": {
      "skipTypeImports": true
    }
  }
}

@jonlepage
Copy link

work if we add type : ex import type { } from ".../";

@mayneyao
Copy link

mayneyao commented Aug 5, 2022

es6: { skipTypeImports: true },
ts: { skipTypeImports: true }

es6 + ts config works for my typescript project

@michal-kurz
Copy link

michal-kurz commented Jan 13, 2023

work if we add type : ex import type { } from ".../";

This seems to be the case 😕 import type { T } from './2' gets omitted, import { T } from './2' does not on 5.0.1.

Unfortunately I'm working on a huge existing React project, and we often import types along with values like this:

import { Component, ComponentProps } from "./Component"

Because of this, our dependency graphs are extremely messy (together with poor dependency flow), and it would be very tedious to manually re-write all type imports into import type xxx. I'll look for trivial solution to automating it, it doesn't sound very likely to me.

@pahen , @realityking do you think it would be feasible to distinguish types and interfaces on the fly, regardless of import syntax? I'm willing to contribute, but unsure if it's worth even digging in :) I think it would at the very least make sense to document this in README.

@PabloLION
Copy link
Collaborator

It seems like a dup of #330, could you confirm please @kamiazya ?

@PabloLION PabloLION added the core dependency tree generation label Jan 29, 2023
@michal-kurz
Copy link

v6 solved my issues, thank you very much 🙏

@joshuakb2
Copy link

v6 solved my issues, thank you very much pray

Are you sure? Still not working for me. I have to specify import type.

@michal-kurz
Copy link

Are you sure? Still not working for me. I have to specify import type.

Yes, I am - I'm sad to hear that isn't working for you. Try with this config, this is what I am using:

const config = {
	fileExtensions: ['js', 'jsx', 'ts', 'tsx'],
	tsConfig: './tsconfig.json',
	detectiveOptions: {
		ts: { skipTypeImports: true },
		tsx: { skipTypeImports: true },
		js: { skipTypeImports: true },
		jsx: { skipTypeImports: true },
		es6: { skipTypeImports: true }
	}
}

@kirbysayshi
Copy link

Not sure if it's better to comment here or on #340, but I've found that the specific type syntax matters:

import { type TheType } from './types'; // is INCLUDED when `skipTypeImports: true`
import type { TheType } from './types';  // is EXCLUDED when `skipTypeImports: true`

I have also tried both syntaxes with the config provided above (#232 (comment)). Only when the using import type { ...} did the following minimum options work for my TS/TSX project:

{
  "detectiveOptions": {
    "ts": {
      "skipTypeImports": true
    },
    "tsx": {
      "skipTypeImports": true
    }
  }
}

For reference, I'm invoking madge using only a small portion of my project, and with command similar to:

pnpm dlx madge --include-npm src/pages/Page1/index.tsx --debug

@lishaduck
Copy link

lishaduck commented Aug 29, 2024

Post TS 5.0, I'd argue that requiring the type modifier is the correct behavior.
While verbatimModuleSyntax isn't on by default for backwards compatibility, it's fair for tooling to require it.
I, for one, would change to be a docs issue:

When searching for circular dependencies with skipTypeImports: true, Madge may have false-positives unless you enable verbatimModuleSyntax in your tsconfig.json file.

For help fixing the issues, see @typescript-eslint/consistent-type-imports and @typescript-eslint/no-import-type-side-effects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core dependency tree generation
Projects
None yet
Development

No branches or pull requests