search

sverweij / dependency-cruiser

Validate and visualize dependencies. Your rules. JavaScript, TypeScript, CoffeeScript. ES6, CommonJS, AMD.
https://npmjs.com/dependency-cruiser
MIT License
5.15k stars 249 forks source link

Why doesn't dependency-cruiser "ignore" my private-registry package like it does with node-modules? #910

Closed bes closed 6 months ago

bes commented 7 months ago

I have a package that is created using wasm-bindgen which is a Rust library for creating WebAssembly libraries. The library is installed through a private registry (GitLab). I don't know what detail is causing dependency-cruiser to complain about my code depending on that library, when it doesn't care about any other package installed into node-modules.

Summary

My question is - what is causing dependency-cruiser to find my private package, when it installed into node-modules.

npx depcruise --config .dependency-cruiser.js app

  error not-in-allowed: some/internal/path/file.tsx → @myorg/mypackage-wasm

The library's package.json looks like this

{
  "name": "@myorg/mypackage-wasm",
  "version": "0.1.0",
  "files": [
    "mypackage_wasm_bg.wasm",
    "mypackage_wasm.js",
    "mypackage_wasm_bg.js",
    "mypackage_wasm.d.ts"
  ],
  "module": "mypackage_wasm.js",
  "types": "mypackage_wasm.d.ts",
  "sideEffects": [
    "./mypackage_wasm.js",
    "./snippets/*"
  ],
  "publishConfig": {
    "registry": "https://gitlab.com/api/v4/projects/11111111/packages/npm/"
  }
}

In package-lock.json it looks something like this:

...
        "@myorg/mypackage-wasm": "0.1.0",
...
    "node_modules/@myorg/mypackage-wasm": {
      "version": "0.10.3",
      "resolved": "https://gitlab.com/api/v4/projects/11111111/packages/npm/@myorg/mypackage-wasm/-/@myorg/mypackage-wasm-0.1.0.tgz",
      "integrity": "sha1-xx"
    },
...
    "@myorg/mypackage-wasm": {
      "version": "0.1.0",
      "resolved": "https://gitlab.com/api/v4/projects/11111111/packages/npm/@myorg/mypackage-wasm/-/@myorg/mypackage-wasm-0.1.0.tgz",
      "integrity": "sha1-xx"
    },
...

(some values have been anonymized)

Context

Environment

sverweij commented 7 months ago

Hi @bes thanks for taking the time to raise this issue. I have the same expectation as you. It shouldn't matter where the modules in node_modules come from. To better understand what is going on I'd need to have a closer look at what is in your .dependency-cruiser.js (or a minimal reproduction example). It's very likely that something in there is the root cause for the difference.

Two guesses based on the information already in the issue:

bes commented 7 months ago

Hi @sverweij, thank you for your kind reply!

Regarding @myorg/mypackage-wasm, here it is, listed with eza -alT from the root of my project (filtering out a lot of files, of course):

drwxr-xr-x      - bes 13 Feb 08:54 .
.rw-r--r--    29k bes 12 Feb 19:16 ├── .dependency-cruiser.js
drwxr-xr-x      - bes 12 Feb 19:17 ├── node_modules
drwxr-xr-x      - bes 12 Feb 20:05 │  ├── @myorg
drwxr-xr-x      - bes 12 Feb 20:05 │  │  └── mypackage-wasm
.rw-r--r--    405 bes 12 Feb 20:05 │  │     ├── package.json
.rw-r--r--   8.6k bes 12 Feb 20:05 │  │     ├── mypackage_wasm.d.ts
.rw-r--r--    186 bes 12 Feb 20:05 │  │     ├── mypackage_wasm.js
.rw-r--r--    42k bes 12 Feb 20:05 │  │     ├── mypackage_wasm_bg.js
.rw-r--r--   721k bes 12 Feb 20:05 │  │     └── mypackage_wasm_bg.wasm
                                   │  ...
.rw-r--r--   1.2M bes 12 Feb 20:05 ├── package-lock.json
.rw-r--r--    16k bes 12 Feb 20:04 ├── package.json

As you noted, yes I am using an allowed section, here is my config (a bit shortened), I've added a bunch of // NOTE where I shortened stuff. Using allowed was the way I found worked best when configuring dependency-cruiser, but if there is a better way I will happily use that.

/** @type {import('dependency-cruiser').IConfiguration} */
module.exports = {
    forbidden: [
        {
            name: "no-orphans",
            comment:
                "This is an orphan module - it's likely not used (anymore?). Either use it or " +
                "remove it. If it's logical this module is an orphan (i.e. it's a config file), " +
                "add an exception for it in your dependency-cruiser configuration. By default " +
                "this rule does not scrutinize dot-files (e.g. .eslintrc.js), TypeScript declaration " +
                "files (.d.ts), tsconfig.json and some of the babel and webpack configs.",
            severity: "warn",
            from: {
                orphan: true,
                pathNot: [
                    "(^|/)\\.[^/]+\\.(js|cjs|mjs|ts|json)$", // dot files
                    "\\.d\\.ts$", // TypeScript declaration files
                    "(^|/)tsconfig\\.json$", // TypeScript config
                    "(^|/)(babel|webpack)\\.config\\.(js|cjs|mjs|ts|json)$", // other configs
                ],
            },
            to: {},
        },
        {
            name: "no-deprecated-core",
            comment:
                "A module depends on a node core module that has been deprecated. Find an alternative - these are " +
                "bound to exist - node doesn't deprecate lightly.",
            severity: "warn",
            from: {},
            to: {
                dependencyTypes: ["core"],
                path: [
                    "^(v8/tools/codemap)$",
                    "^(v8/tools/consarray)$",
                    "^(v8/tools/csvparser)$",
                    "^(v8/tools/logreader)$",
                    "^(v8/tools/profile_view)$",
                    "^(v8/tools/profile)$",
                    "^(v8/tools/SourceMap)$",
                    "^(v8/tools/splaytree)$",
                    "^(v8/tools/tickprocessor-driver)$",
                    "^(v8/tools/tickprocessor)$",
                    "^(node-inspect/lib/_inspect)$",
                    "^(node-inspect/lib/internal/inspect_client)$",
                    "^(node-inspect/lib/internal/inspect_repl)$",
                    "^(async_hooks)$",
                    "^(punycode)$",
                    "^(domain)$",
                    "^(constants)$",
                    "^(sys)$",
                    "^(_linklist)$",
                    "^(_stream_wrap)$",
                ],
            },
        },
        {
            name: "not-to-deprecated",
            comment:
                "This module uses a (version of an) npm module that has been deprecated. Either upgrade to a later " +
                "version of that module, or find an alternative. Deprecated modules are a security risk.",
            severity: "warn",
            from: {},
            to: {
                dependencyTypes: ["deprecated"],
            },
        },
        {
            name: "no-non-package-json",
            severity: "error",
            comment:
                "This module depends on an npm package that isn't in the 'dependencies' section of your package.json. " +
                "That's problematic as the package either (1) won't be available on live (2 - worse) will be " +
                "available on live with an non-guaranteed version. Fix it by adding the package to the dependencies " +
                "in your package.json.",
            from: {},
            to: {
                dependencyTypes: ["npm-no-pkg", "npm-unknown"],
            },
        },
        {
            name: "no-duplicate-dep-types",
            comment:
                "Likely this module depends on an external ('npm') package that occurs more than once " +
                "in your package.json i.e. bot as a devDependencies and in dependencies. This will cause " +
                "maintenance problems later on.",
            severity: "warn",
            from: {},
            to: {
                moreThanOneDependencyType: true,
                // as it's pretty common to have a type import be a type only import
                // _and_ (e.g.) a devDependency - don't consider type-only dependency
                // types for this rule
                dependencyTypesNot: ["type-only"],
            },
        },

        /* rules you might want to tweak for your specific situation: */
        {
            name: "not-to-spec",
            comment:
                "This module depends on a spec (test) file. The sole responsibility of a spec file is to test code. " +
                "If there's something in a spec that's of use to other modules, it doesn't have that single " +
                "responsibility anymore. Factor it out into (e.g.) a separate utility/ helper or a mock.",
            severity: "error",
            from: {},
            to: {
                path: "\\.(spec|test)\\.(js|mjs|cjs|ts|ls|coffee|litcoffee|coffee\\.md)$",
            },
        },
        {
            name: "not-to-dev-dep",
            severity: "error",
            comment:
                "This module depends on an npm package from the 'devDependencies' section of your " +
                "package.json. It looks like something that ships to production, though. To prevent problems " +
                "with npm packages that aren't there on production declare it (only!) in the 'dependencies'" +
                "section of your package.json. If this module is development only - add it to the " +
                "from.pathNot re of the not-to-dev-dep rule in the dependency-cruiser configuration",
            from: {
                path: "^(./)",
                pathNot: "\\.(spec|test)\\.(js|mjs|cjs|ts|ls|coffee|litcoffee|coffee\\.md)$",
            },
            to: {
                dependencyTypes: ["npm-dev"],
            },
        },
        {
            name: "optional-deps-used",
            severity: "info",
            comment:
                "This module depends on an npm package that is declared as an optional dependency " +
                "in your package.json. As this makes sense in limited situations only, it's flagged here. " +
                "If you're using an optional dependency here by design - add an exception to your" +
                "dependency-cruiser configuration.",
            from: {},
            to: {
                dependencyTypes: ["npm-optional"],
            },
        },
        {
            name: "peer-deps-used",
            comment:
                "This module depends on an npm package that is declared as a peer dependency " +
                "in your package.json. This makes sense if your package is e.g. a plugin, but in " +
                "other cases - maybe not so much. If the use of a peer dependency is intentional " +
                "add an exception to your dependency-cruiser configuration.",
            severity: "warn",
            from: {},
            to: {
                dependencyTypes: ["npm-peer"],
            },
        },
    ],
    allowed: [
        {
            from: {
                path: "/",
            },
            to: {
                path: ["^events", "^stream", "^buffer"],
            },
        },
        {
            from: {
                path: "app/",
            },
            to: {
                path: [
                    "^app/",
                    "^lib-1/",
                    "^lib-2/",
                    "^lib-3/",
                    "^lib-4/",
                    "^lib-5/",
                    // NOTE 10 more lines in the real file
                    // Trying to get rid of this
                    "^@myorg/mypackage-wasm($|/)",
                ],
            },
        },
        {
            from: {
                path: "app2",
            },
            to: {
                path: [
                    // NOTE 10 more lines in the real file
                ],
            },
        },
        {
            from: {
                path: "app3",
            },
            to: {
                path: [
                    // NOTE 11 more lines in the real file
                ],
            },
        },

        // NOTE 23 more config blocks in the real file

    ],
    allowedSeverity: "error",
    options: {
        /*
         conditions specifying which files not to follow further when encountered:
         - path: a regular expression to match
         - dependencyTypes: see https://github.com/sverweij/dependency-cruiser/blob/master/doc/rules-reference.md#dependencytypes-and-dependencytypesnot
         for a complete list
         */
        doNotFollow: {
            path: "node_modules",
        },

        /*
         conditions specifying which dependencies to exclude
         - path: a regular expression to match
         - dynamic: a boolean indicating whether to ignore dynamic (true) or static (false) dependencies.
         leave out if you want to exclude neither (recommended!)
         */
        exclude: {
            path: [
                "node_modules",
                "^playwright/",
                "^cloud-infrastructure/",
                "^test/",
                "^app/build.ts",
                "build/webpack/",
                // NOTE 17 more lines in the real file
            ],
        },

        /*
         pattern specifying which files to include (regular expression)
         dependency-cruiser will skip everything not matching this pattern
         */
        // includeOnly : '',

        /*
         dependency-cruiser will include modules matching against the focus
         regular expression in its output, as well as their neighbours (direct
         dependencies and dependents)
        */
        // focus : '',

        /* list of module systems to cruise */
        // moduleSystems: ['amd', 'cjs', 'es6', 'tsd'],

        /*
         prefix for links in html and svg output (e.g. 'https://github.com/you/yourrepo/blob/develop/'
         to open it on your online repo or `vscode://file/${process.cwd()}/` to
         open it in visual studio code),
        */
        // prefix: '',

        /*
         false (the default): ignore dependencies that only exist before typescript-to-javascript compilation
         true: also detect dependencies that only exist before typescript-to-javascript compilation
         "specify": for each dependency identify whether it only exists before compilation or also after
        */
        tsPreCompilationDeps: "specify",

        /* list of extensions (typically non-parseable) to scan. Empty by default. */
        // extraExtensionsToScan: [".json", ".jpg", ".png", ".svg", ".webp"],

        /*
         if true combines the package.jsons found from the module up to the base
         folder the cruise is initiated from. Useful for how (some) mono-repos
         manage dependencies & dependency definitions.
        */
        // combinedDependencies: false,

        /* if true leave symlinks untouched, otherwise use the realpath */
        // preserveSymlinks: false,

        /*
         TypeScript project file ('tsconfig.json') to use for
         (1) compilation and
         (2) resolution (e.g. with the paths property)

         The (optional) fileName attribute specifies which file to take (relative to
         dependency-cruiser's current working directory). When not provided
         defaults to './tsconfig.json'.
        */
        tsConfig: {
            fileName: "tsconfig.json",
        },

        /*
         Webpack configuration to use to get resolve options from.

         The (optional) fileName attribute specifies which file to take (relative
         to dependency-cruiser's current working directory. When not provided defaults
         to './webpack.conf.js'.

         The (optional) `env` and `args` attributes contain the parameters to be passed if
         your webpack config is a function and takes them (see webpack documentation
         for details)
        */
        webpackConfig: {
            fileName: "./webpack-util/webpack.alias.js",
        },

        /* options to pass on to enhanced-resolve, the package dependency-cruiser
       uses to resolve module references to disk. You can set most of these
       options in a webpack.conf.js - this section is here for those
       projects that don't have a separate webpack config file.

       Note: settings in webpack.conf.js override the ones specified here.
     */
        enhancedResolveOptions: {
            /* List of strings to consider as 'exports' fields in package.json. Use
         ['exports'] when you use packages that use such a field and your environment
         supports it (e.g. node ^12.19 || >=14.7 or recent versions of webpack).

        If you have an `exportsFields` attribute in your webpack config, that one
         will have precedence over the one specified here.
      */
            exportsFields: ["exports"],
            /* List of conditions to check for in the exports field. e.g. use ['imports']
         if you're only interested in exposed es6 modules, ['require'] for commonjs,
         or all conditions at once `(['import', 'require', 'node', 'default']`)
         if anything goes for you. Only works when the 'exportsFields' array is
         non-empty.

        If you have a 'conditionNames' attribute in your webpack config, that one will
        have precedence over the one specified here.
      */
            conditionNames: ["import", "require", "node", "default"],
        },
        reporterOptions: {
            dot: {
                /*
                 pattern of modules that can be consolidated in the detailed
                 graphical dependency graph. The default pattern in this configuration
                 collapses everything in node_modules to one folder deep so you see
                 the external modules, but not the innards your app depends upon.
                 */
                collapsePattern: [
                    "node_modules/[^/]+",
                    "^app/",
                    // NOTE 18 more lines in the real file
                ],
            },
            archi: {
                /*
                 pattern of modules that can be consolidated in the high level
                 graphical dependency graph. If you use the high level graphical
                 dependency graph reporter (`archi`) you probably want to tweak
                 this collapsePattern to your situation.
                 */
                collapsePattern: "^(packages|src|lib|app|bin|test(s?)|spec(s?))/[^/]+|node_modules/[^/]+",
            },
            text: {
                highlightFocused: true,
            },
        },
    },
};
sverweij commented 7 months ago

Hi @bes it's a matter of (lack of) module resolution. The @myorg/mypackage-wasm has a module field in its manifest that functions as the main entry point. For dependency-cruiser to look at this filed, it currently still has to be specified in the resolve option (attribute mainFields). As of version 14.1.0 --init ensures this is done for you.

The dependency-cruiser config you supplied above (thanks!) indeed doesn't have this, adding it will fix the issue:

    resolve: {
       enhancedResolveOptions: {
        exportsFields: ["exports"],
        conditionNames: ["import", "require", "node", "default"],
        // added this line:
        mainFields: ["module", "main", "types", "typings"],
    },

Regarding the allowed vs forbidden - I think it the rules can be reformulated as as forbidden, become easier to maintain (and give more targeted error messages). I'll try to explain in a separate comment later.

bes commented 6 months ago

Thanks! That worked well! Still need to convert to forbidden, and would be very happy for any help I could get.

sverweij commented 6 months ago

Hi @bes

As an example, this part of the 'allowed' rule ("dependencies between app, a few libs and within app itself are allowed")

    {
      from: {
        path: "app/",
      },
      to: {
        path: [
          "^app/",
          "^lib-1/",
          "^lib-2/",
          "^lib-3/",
          "^lib-4/",
          "^lib-5/",
          // NOTE 10 more lines in the real file
          // Trying to get rid of this
          "^@myorg/mypackage-wasm($|/)",
        ],
      },
    },

... can also be written as "it's forbidden to depend on anything except app itself and libs".

    {
      name: "app-only-to-self-or-lib",
      severity: "error",
      from: {
        path: "app/",
      },
      to: {
        pathNot: [
          "^app/",
          "^lib-1/",
          "^lib-2/",
          "^lib-3/",
          "^lib-4/",
          "^lib-5/",
          // NOTE 10 more lines in the real file
          // Trying to get rid of this
          "^@myorg/mypackage-wasm($|/)",
        ],
      },
    },
github-actions[bot] commented 6 months ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.