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

File with a fake circular dependency to itself #271

Open
unpollito opened this issue Mar 4, 2021 · 13 comments
Open

File with a fake circular dependency to itself #271

unpollito opened this issue Mar 4, 2021 · 13 comments
Labels
bug core dependency tree generation

Comments

@unpollito
Copy link

unpollito commented Mar 4, 2021

I've noticed a strange behavior with madge. It turns out that I have a file called auth0.ts, which acts as a facade for Auth0, which starts like this:

import * as Auth0 from "auth0";

This is not a circular dependency, as I have installed the auth0 package via npm, and it shows up in my package.json. The import works as expected, giving me access to the content of the auth0 package. However, running npx madge -c auth0.ts still complains:

Processed 1 file (608ms) (1 warning)

✖ Found 1 circular dependency!

1) auth0.ts
@mssngr
Copy link

mssngr commented Mar 4, 2021

I have this same issue with an absolute path named utils and any other file named utils

// src/components/shared/utils.ts
import { myFunc } from 'utils'

This results in an image like this:
Screen Shot 2021-03-04 at 6 49 58 AM

Workaround is to use a relative path, for now.

@mrjoelkemp
Copy link
Contributor

Hey! Does it still happen if you set includeNpm to true: https://github.com/pahen/madge#configuration?

@unpollito
Copy link
Author

Hey @mrjoelkemp, thanks for chipping in! I can confirm I'm still hitting the same issue with includeNpm: true in the config.

@mrjoelkemp
Copy link
Contributor

Thanks for confirming. We just delegate to the typescript compiler for module path resolution: https://github.com/dependents/node-filing-cabinet/blob/master/index.js#L145-L153. Are you supplying a typescript config? If so, perhaps there's a relevant configuration option you can set.

@patroza
Copy link

patroza commented May 1, 2021

I run into this issue too, it appears like it's trying to use the classic moduleResolution, even though I have set node

@patroza
Copy link

patroza commented May 1, 2021

The problem occurs when using extends. If the moduleResolution is changed in the base config, but not in the selected config file, then the problem occurs.

The problem seems to stem from the way how compilerOptions are loaded and changed in Madge; I think the solution should be either:
a) let typescript handle the tsconfig by passing it on create of the compiler service, so it will look for the extends and processes it accordingly - but then how to make the Madge desired overrides?
or
b) handle the extends and merging of compilerOptions accordingly in Madge.

@alvaro-cuesta
Copy link

alvaro-cuesta commented May 27, 2021

This also happened to us even though we're not using TS. I have a module on redux/index.js (imported elsewhere as ./redux) that acts as a facade to Redux. The first line is import { createStore, combineReducers } from 'redux' which I'm guessing is the one detected as a circular dependency.

Using it as CLI on v4.0.2.

@PabloLION PabloLION added bug core dependency tree generation labels Jan 29, 2023
@FabianFrank
Copy link

This happens to me as well. If you have a file named foo.ts that has an import from "foo" despite "foo" being an npm module madge will act as if the file imports itself.

@PabloLION
Copy link
Collaborator

Yea I guess madge parsed "foo" as "./foo" which is incorrect. Will look into this much later.

@mx-bernhard
Copy link

mx-bernhard commented Feb 11, 2023

I had the same problem and found out, that supplying madge the path to the tsconfig instead of the tsconfig object causes madge (or a dependency of it) to provide its dependencies a tsconfig object that does not contain strings for compilerOptions.module and compilerOptions.moduleResolution respectively (we are using esnext and node for those) but numbers. The receiving side then tries to find the corresponding (canonicalized?) value and sees a number and removes it, which in turn results in undefined for those two values. As a workaround I used get-tsconfig like this which is capable of ignoring comments in a tsconfig:

  import { getTsconfig } from 'get-tsconfig';

  const { config } = getTsconfig(tsConfigPath);
  const madgeResult = await madge(path.join(__dirname, './src'), {
    tsConfig: config,
  });

Supplying the object instead of the path seems to not cause the aforementioned removal of those options.

This not only fixed the problems mentioned in this issue, but various missing resolved dependencies to directories containing an index.ts

EDIT:
Obviously we are not using the cli version of madge, but the npm package in a .js script.

@mx-bernhard
Copy link

This seems to be the culprit. It causes property-values of the wrong format (numbers instead of strings) to be written over the correct ones. Maybe just pick the values that are actually needed or exclude the ones, that can already be found in the target?

@homer0
Copy link

homer0 commented Mar 27, 2023

Hi 👋

I was implementing madge in a project today, and I found this issue. I followed the chain of calls and the problem is what @mx-bernhard mentioned:

Let's say you have a tsconfig.json with these props:

{
  "compilerOptions": {
    "target": "es5",
    "module": "esnext",
    "moduleResolution": "node"
  }
}

If you run madge via CLI and send --ts-config tsconfig.json, you'll end up in this if, which will read the tsconfig.json and format its compilerOptions using TypeScript's parseJsonSourceFileConfigFileContent, which will replace those string values with (probably) enum indexes:

{
  "compilerOptions": {
    "target": 1,
    "module": 99,
    "moduleResolution": 2
  }
}

Now, the problem is that the generated object (with numbers instead of strings) is what madge sends down to its dependencies, specially filing-cabinet.

Inside filing-cabinet, when trying to resolve a path a for ts files, we end up in this check, which runs TypeScript's convertCompilerOptionsFromJson, and yes, that function expects strings, not numbers, so the result is

{
  "compilerOptions": {
    "target": undefined,
    "module": undefined,
    "moduleResolution": undefined
  }
}

@homer0
Copy link

homer0 commented Mar 27, 2023

For anyone interested, I just made a small workaround to be able to use CLI:

Since madge uses the rc package, and as @mx-bernhard pointed on their comment, "Supplying the object instead of the path seems to not cause the aforementioned removal of those options.", I made a shell script that would read the tsconfig.json, put it on a .madgerc and then run the CLI.

#!/bin/sh -e

TS_CONFIG=$(node -e "console.log(JSON.stringify(require('get-tsconfig').getTsconfig().config, null, 2))")

JSON_STRING=$(cat <<EOF
{
  "_note_": "This file is autogenerated",
  "tsConfig": $TS_CONFIG,
  "fileExtensions": ["js", "ts", "tsx"]
}
EOF
)

echo "$JSON_STRING" > .madgerc
madge ./src -c --warning
  • This is called from a script in the package.json, but if you indent to run it directly, you should prefix the call to madge with pnpm exec/npx/yarn dlx/whatever.
  • Don't forget to chmod +x

(I really like the ora UI :P)

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

9 participants