pahen / madge

Create graphs from your CommonJS, AMD or ES6 module dependencies
MIT License
8.95k stars 318 forks source link

[BUG] Some path which omits '/index' should not be skipped #371

Closed feng9017 closed 5 months ago

feng9017 commented 1 year ago

I found that some dependency ../store/mp was ignored by Madge 6.0.0 . But the dependency is clearly stated. I checked it by use '--debug' mode, found this:

tree skipping an empty filepath resolution for partial: ../store/mp +1ms

Actually, the full paths should be ../store/mp/index, but eslint will give a warning like this :

Useless path segments for "../store/mp/index", should be "../store/mp"eslint[import/no-useless-path-segments](https://github.com/import-js/eslint-plugin-import/blob/v2.27.5/docs/rules/no-useless-path-segments.md)

import/no-useless-path-segments is a common eslint plugin. I think it is necessary to check somePath/index when Madge find that somePath is empty.

Alegiter commented 1 year ago

+1

Adding "/index" to some path resolves madge skipping files

I have file extract.test.ts with imports:

import { ... } from "../../model"
import { ... } from "./extract"

And my madge result with --json for this file is:

{
  "call.ts": [],
  "extract.test.ts": [
    "extract.ts"
  ],
  "extract.ts": [
    "call.ts"
  ]
}

But if I change model import for ../../model/index:

{
  "../../model/***.ts": [],
  "../../model/index.ts": [
    "../../model/***.ts",
  ],
  "call.ts": [],
  "extract.test.ts": [
    "../../model/index.ts",
    "extract.ts"
  ],
  "extract.ts": [
    "call.ts"
  ]
}
aleksanderd commented 8 months ago

same here, index.ts files seems to be ignored: all imports of dirs with index.ts are skipped(

is there any way to fix it?

Jym77 commented 8 months ago

Facing the same issue and digging a bit. ~I think it is a problem with dependency-tree, likely https://github.com/dependents/node-dependency-tree/issues/118, which Madge uses to get its dependencies. At least the "skipping" message comes from there.~

Trying to dig further into dependency-tree to see whether this is just an option to pass or a deeper issue…

Jym77 commented 8 months ago

OK, I think this is a dependency-tree problem that was fixed in v10, madge is still running v9 and thus has the problem.

I'm not really able to see which update of dependency-tree did the trick, but it seems to be the root cause… https://github.com/dependents/node-dependency-tree/releases/tag/v10.0.0

Jym77 commented 8 months ago

…which should make it easy to fix 😁 The latest release of madge, v6.1.0 is from June '23. dependency-tree has been updated to v10.0.9 in October '23.

So, I assume that simply making a new release would solve the problem… <= @pahen

bestickley commented 7 months ago

@Jym77, I ran into this same issue and I used package.json#pnpm.overrides to enforce dependency-tree@10.0.9 to be used by madge and it still did not work for me :( Import identifiers that had implicit "index" files still showed up as warnings in madge because they could not be resolved. Example output of madge with dependency-tree@10:

pnpx madge --warning --circular ./src/**/*
 WARN  1 deprecated subdependencies found: flatten@1.0.3
Packages: +179
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Progress: resolved 178, reused 178, downloaded 0, added 179, done
Processed 57 files (1.2s) (15 warnings)

✔ No circular dependency found!

✖ Skipped 15 files

drizzle-orm/postgres-js
drizzle-orm
postgres
@aws-sdk/rds-signer
@aws-sdk/client-secrets-manager
drizzle-orm/pg-core
@aws-lambda-powertools/logger
drizzle-kit
drizzle-orm/postgres-js/migrator
../adapters/secondary
./types
zod
../base
./adapters/secondary

Then if I update all import specifiers with /index:

pnpx madge --warning --circular ./src/**/*

> @lh/core@0.1.0 check-circular-dependencies /Users/stickb/Code/dos/lighthouse/core
> madge --circular --warning src/**/*

Processed 40 files (1s) (10 warnings)

✖ Found 1 circular dependency!

1) adapters/secondary/db-adapter.ts > modules/new-user/index.server.ts > modules/new-user/update-new-user.usecase.ts > modules/new-user/new-user.repo.ts > adapters/secondary/index.ts

✖ Skipped 10 files

postgres
@aws-sdk/rds-signer
@aws-sdk/client-secrets-manager
drizzle-kit
drizzle-orm/postgres-js
drizzle-orm/postgres-js/migrator
drizzle-orm
zod
drizzle-orm/pg-core
@aws-lambda-powertools/logger

Is there anything else you did to get it to work?

Jym77 commented 7 months ago

Is there anything else you did to get it to work?

Since I am only at the "experiment and play" step, I brutally edited madge.js in my node_modules to accept a DependencyTree parameter instead of using its own, installed 10.0.9 locally, and injected it that way. Not my proudest hack 🙈 And definitely won't work once I try to actually use this… Or when try to use madge from command line rather than a script.

kirbysayshi commented 7 months ago

I can confirm that upgrading dependency-tree >= 10 fixes the issue! Madge 6.1.0. Since I'm using pnpm I did the following in my package.json:

 "overrides": {
      "madge>dependency-tree@<10": "10"
    }

I went from 1000s of skipped files to 7!

Jym77 commented 7 months ago

For yarn, the workaround in package.json is:

"resolutions": {
    "dependency-tree": "^10.0.9"
  }
Vovan-VE commented 6 months ago

Thanks for workaround! npm:

  "overrides": {
    "madge": {
      "dependency-tree": "^10"
    }
  },
Jym77 commented 5 months ago

FYI, updating to madge 7.0.0 fixed this for me (no need for custom resolution anymore). The correct dependency-tree is now a dependency 🎉

pahen commented 5 months ago

FYI, updating to madge 7.0.0 fixed this for me (no need for custom resolution anymore). The correct dependency-tree is now a dependency 🎉

Nice :)