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

False positives when folder name equals node_module name #306

Open
Eazymov opened this issue Jan 11, 2022 · 3 comments
Open

False positives when folder name equals node_module name #306

Eazymov opened this issue Jan 11, 2022 · 3 comments
Labels
bug core dependency tree generation

Comments

@Eazymov
Copy link

Eazymov commented Jan 11, 2022

Hi!

Madge version: 5.0.1

Steps to reproduce:

  1. Create a project with this very simple structure:
    src/
    --react/
    ----a.js
    ----index.js
  2. Fill in the files
// src/react/a.js
import * as React from "react";
// src/react/index.js
import { a } from "./a";
  1. Run madge src --circular
    Error:
    image

Adding "react" dependency doesn't change anything

@lobsterkatie
Copy link

lobsterkatie commented Feb 16, 2022

I am also running into this, though in my case my structure looks like this:

src/
  index.server.ts
  config
    webpack.ts
    types.ts
// index.server.ts
export { SomeOtherType } from "./config/webpack";

// config/webpack.ts
import { SomeOtherType } from "./types";

// config/types.ts
import { SomeType } from 'webpack';
export type SomeOtherType = Record<string, any>;

and I am getting

⠋ Finding files⠙ ./index.server.ts⠹ config/types.ts⠸ config/webpack.ts Found 1 circular dependency!

in our Madge GitHub Action.

Interestingly, it was adding the export to index.server.ts which triggered the error. The rest has always passed just fine.(In case it helps, here's the PR: getsentry/sentry-javascript#4597)

EDIT: When I run madge locally, it only lists the last two files (not index.server.js, even though changing it is what triggered the problem).

image

Running it with --debug shows the problem:

image

The second-to-last line shouldn't be src/config/webpack.ts, it should be node_modules/webpack/lib/webpack.js. IOW, madge isn't differentiating between import { thing } from 'stuff' and import { thing } from './stuff'.

lobsterkatie added a commit to getsentry/sentry-javascript that referenced this issue Feb 16, 2022
…dependency check (#4598)

The library we use to detect circular dependencies, `madge`, has a bug wherein local files whose names match node modules cause false positives. (For more details, see pahen/madge#306.) In our case, it was triggered by #4597, which added an export from  file (`config/types.ts`) which depends on the node module `webpack` and which is depended upon by the local file `config/webpack.ts`.

To solve this for the moment, this excludes the problematic file from the check.
@wolfgang42
Copy link

Seems to happen with files, too, not just folders; I have a knex.ts:

import {knex} from 'knex'
export default knex(require('./knexfile'))

... and madge --circular reports it as a dep of itself:

✖ Found 1 circular dependency!

1) knex.ts

@lobsterkatie
Copy link

lobsterkatie commented Nov 21, 2022

Seems to happen with files, too, not just folders
... and madge --circular reports it as a dep of itself

I just ran into this, too, having forgotten that this was something I needed to worry about. Any progress here? Can we do anything to help move this fix along? (UPDATE: Just found #334. TL;DR - Life happens, so we probably won't see any movement here until early next year.)

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

No branches or pull requests

4 participants