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

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

Closed
feng9017 opened this issue May 6, 2023 · 12 comments
Closed

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

feng9017 opened this issue May 6, 2023 · 12 comments

Comments

@feng9017
Copy link

feng9017 commented May 6, 2023

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
Copy link

+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
Copy link

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
Copy link

Jym77 commented Jan 29, 2024

Facing the same issue and digging a bit.
I think it is a problem with dependency-tree, likely dependents/node-dependency-tree#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
Copy link

Jym77 commented Jan 29, 2024

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

  • Adding a console.log statement to see the result of the call to DependencyTree at https://github.com/pahen/madge/blob/master/lib/tree.js#L121, I do not get the dependency:
    {
      '/mnt/c/Projects/a11y/alfa/packages/alfa-css/src/calculation/index.ts': {}
    }
    
  • Calling, with the same config, DependencyTree directly, and showing the result,
    • with v10.0.1 to v10.0.9 (somehow, v10.0.0 crashes), I get the dependency:
      {
        '/mnt/c/Projects/a11y/alfa/packages/alfa-css/src/calculation/index.ts': {
          '/mnt/c/Projects/a11y/alfa/packages/alfa-css/src/calculation/math-expression/index.ts': {
            '/mnt/c/Projects/a11y/alfa/packages/alfa-css/src/calculation/math-expression/math.ts': [Object]
          }
        }
      }
      
    • same with v9.0.0 (no other v9.* version) of dependency-tree, I do not get the dependency:
      {
        '/mnt/c/Projects/a11y/alfa/packages/alfa-css/src/calculation/index.ts': {}
      }
      

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
Copy link

Jym77 commented Jan 29, 2024

…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
Copy link

bestickley commented Feb 15, 2024

@Jym77, I ran into this same issue and I used package.json#pnpm.overrides to enforce [email protected] 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: [email protected]
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/[email protected] 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
Copy link

Jym77 commented Feb 15, 2024

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
Copy link

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
Copy link

Jym77 commented Mar 4, 2024

For yarn, the workaround in package.json is:

"resolutions": {
    "dependency-tree": "^10.0.9"
  }

@Vovan-VE
Copy link

Vovan-VE commented Mar 8, 2024

Thanks for workaround!
npm:

  "overrides": {
    "madge": {
      "dependency-tree": "^10"
    }
  },

@Jym77
Copy link

Jym77 commented Apr 30, 2024

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
Copy link
Owner

pahen commented Apr 30, 2024

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 :)

@pahen pahen closed this as completed Apr 30, 2024
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

No branches or pull requests

8 participants