Closed boneskull closed 8 months ago
sverweij
commented
9 months ago Hi @boneskull thanks for raising this issue. Worried, I created a minimal repro repo but couldn't reproduce the problem. This means there's a difference in setup between that repo and the repo you have at hand. Could you share of that repo
options section is the interesting bit, if you don't want to share the rules setup)sverweij
commented
9 months ago b.t.w. I've checked out boneskull/midnight-smoker and run dependency-cruiser against it, and there I can't reproduce the issue either. For reference I've included the .dependency-cruiser.js I've used (placed in the root of the monorepo - command used: npx depcruise packages.
/** @type {import('dependency-cruiser').IConfiguration} */
module.exports = {
forbidden: [
{
name: 'no-circular',
severity: 'warn',
comment:
'This dependency is part of a circular relationship. You might want to revise ' +
'your solution (i.e. use dependency inversion, make sure the modules have a single responsibility) ',
from: {},
to: {
circular: true,
},
},
{
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: 'not-to-unresolvable',
comment:
"This module depends on a module that cannot be found ('resolved to disk'). If it's an npm " +
'module: add it to your package.json. In all other cases you likely already know what to do.',
severity: 'error',
from: {},
to: {
couldNotResolve: true,
},
},
{
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)$',
},
},
{
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: '^(packages)',
pathNot: [
'(spec|test)\\.(js|mjs|cjs|ts)$',
'packages/[^/]+/test',
'packages/[^/]+/scripts',
],
},
to: {
dependencyTypes: ['npm-dev'],
pathNot: ['^node_modules/@types/'],
},
},
{
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'],
},
},
],
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/main/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: '',
// dynamic: true
// },
/* 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: 'https://github.com/boneskull/midnight-smoker/blob/main/',
/* 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
*/
// set to false for midnight-smoker as in the pre-compilation deps there's quite
// a few circular dependencies that only distract at the moment
tsPreCompilationDeps: false,
/*
list of extensions to scan that aren't javascript or compile-to-javascript.
Empty by default. Only put extensions in here that you want to take into
account that are _not_ parsable.
*/
// 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.
*/
// set to true because packages/midnight-smoker does use dependencies
// declared in the root package.json. B.t.w. when switching this back to
// false dependency-cruiser will still find the dependencies, but will
// flag them as undeclared.
combinedDependencies: true,
/* 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',
},
/* 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'],
/*
The extensions, by default are the same as the ones dependency-cruiser
can access (run `npx depcruise --info` to see which ones that are in
_your_ environment. If that list is larger than what you need (e.g.
it contains .js, .jsx, .ts, .tsx, .cts, .mts - but you don't use
TypeScript you can pass just the extensions you actually use (e.g.
[".js", ".jsx"]). This can speed up the most expensive step in
dependency cruising (module resolution) quite a bit.
*/
// extensions: [".js", ".jsx", ".ts", ".tsx", ".d.ts"],
/*
If your TypeScript project makes use of types specified in 'types'
fields in package.jsons of external dependencies, specify "types"
in addition to "main" in here, so enhanced-resolve (the resolver
dependency-cruiser uses) knows to also look there. You can also do
this if you're not sure, but still use TypeScript. In a future version
of dependency-cruiser this will likely become the default.
*/
mainFields: ['main', 'types', 'typings'],
},
progress: {
type: 'cli-feedback',
},
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/(@[^/]+/[^/]+|[^/]+)',
/* Options to tweak the appearance of your graph.See
https://github.com/sverweij/dependency-cruiser/blob/main/doc/options-reference.md#reporteroptions
for details and some examples. If you don't specify a theme
don't worry - dependency-cruiser will fall back to the default one.
*/
theme: {
graph: {
/* use splines: "ortho" for straight lines. Be aware though
graphviz might take a long time calculating ortho(gonal)
routings. If it becomes too slow use "true" here; the lines
will be less pretty but rendering times will be significantly
shorter
*/
splines: 'ortho',
},
modules: [
{
criteria: {matchesFocus: true},
attributes: {
fillcolor: 'lime',
penwidth: 2,
},
},
{
criteria: {matchesFocus: false},
attributes: {
fillcolor: 'lightgrey',
},
},
{
criteria: {matchesReaches: true},
attributes: {
fillcolor: 'lime',
penwidth: 2,
},
},
{
criteria: {matchesReaches: false},
attributes: {
fillcolor: 'lightgrey',
},
},
{
criteria: {source: '/pm/'},
attributes: {fillcolor: '#ccccff'},
},
{
criteria: {source: '/rules/'},
attributes: {fillcolor: '#ccffcc'},
},
],
dependencies: [
{
criteria: {'rules[0].severity': 'error'},
attributes: {fontcolor: 'red', color: 'red'},
},
{
criteria: {'rules[0].severity': 'warn'},
attributes: {fontcolor: 'orange', color: 'orange'},
},
{
criteria: {'rules[0].severity': 'info'},
attributes: {fontcolor: 'blue', color: 'blue'},
},
{
criteria: {resolved: '/pm/'},
attributes: {color: '#0000ff77'},
},
{
criteria: {resolved: '/rules/'},
attributes: {color: '#00770077'},
},
],
},
},
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/(@[^/]+/[^/]+|[^/]+)',
/* Options to tweak the appearance of your graph.See
https://github.com/sverweij/dependency-cruiser/blob/main/doc/options-reference.md#reporteroptions
for details and some examples. If you don't specify a theme
for 'archi' dependency-cruiser will use the one specified in the
dot section (see above), if any, and otherwise use the default one.
*/
// theme: {
// },
},
text: {
highlightFocused: true,
},
},
},
};
// generated: dependency-cruiser@15.3.0 on 2023-11-18T12:27:17.162Z
Per the docs, it sounds like module resolution is done via webpack? If it was possible to do using Node.js' own facilities, I imagine this would work. It could be that webpack just doesn't understand the exports field in yargs.
It's indeed done with enhanced-resolve, which happens to be the resolver webpack uses as well. enhanced-resolve does understand exports fields, as well as imports, main, browser and types fields. It's even configurable to look into fields if someone thinks module resolution in javascript land isn't complicated enough yet.
Out of the box dependency-cruiser configures enhanced-resolve for most common use cases. The default configuration generated with --init adds a few others on top of that as well (in the configuration instead of in the core to preserve backwards compatibility).
Dependency-cruiser uses enhanced-resolve instead of the node native resolver so it can work with all the interesting extensions authors of babel, typescript, webpack, parcel, coffee-script etc have bolted on top of (or as alternatives to) node, ranging from path mappings to completely new languages. So far enhanced-resolve has proven to be very reliable as well as sufficiently flexible. I have yet to find a case where it doesn't work at least as well as nodejs' own resolver.
github-actions[bot]
commented
9 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.
boneskull
commented
9 months ago Yeah, I can reproduce it still. This is what I did:
midnight-smoker, branch main (39e62839ee52c79cf0109cbe842c1dfb4f3419e2)rm -rf node_modules packages/*/node_modulesnpm inpx dependency-cruiser -- --initnpx dependency-cruiser -- .It complains about some "orphans" because it is not fully configured, but otherwise, the errors about yargs are still there.
sverweij
commented
9 months ago Hi @boneskull thanks for the update
rm -rf node_modules packages/*/node_modules ? That will effectively remove the yargs node module (and all other packages installed from npm), which will make it hard to find indeed.npx dependency-cruiser without installing it locally first it will run from the magic spot npm decides to install dependency-cruiser where likely the typescript compiler isn't available (or available with a wildly different version number). I've included a warning in the command line version against global installs, but apparently it doesn't work for "npx 'without installing'" runs, so I'll see if I can add that for those as well.B.t.w. the second point doesn't matter for the issue at hand. I've tried it with npx dependency-cruiser -- . and cannot reproduce the issue (but then again I didn't delete the node_modules yargs references should resolve to).
This is what the warning normally looks like when running dependency-cruiser from a global install:
WARNING: You're running a globally installed dependency-cruiser. We recommend to install and run it as a local devDependency in your project instead. There it has your project's environment and transpilers at its disposal. That will ensure it can find e.g. TypeScript, Vue or Svelte modules and dependencies.```
github-actions[bot]
commented
8 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.
boneskull
commented
7 months ago @sverweij
Re: 1 - The rm -rf was followed by an npm install. I didn't want to install exclusively from package-lock.json, so I didn't use npm ci.
Re: 2 - npx doesn't always imply "global". If dependency-cruiser is installed in your local project, it mimics the npm exec behavior. So I could have written it as npm exec, but I think I exec is a relatively new npm command. Regardless: it runs node_modules/.bin/depcruise
(Apologies for not getting back to this more quickly; I am very poor at keeping on top of GH notifications)
Yargs is doing weird things, and dependency-cruiser doesn't understand it. At least one of those things is a deep corner case that probably isn't worth addressing. Maybe fun to think about?
Expected Behavior
Should not get a
not-to-unresolvableerror from recommended yargs usage.Current Behavior
These two requires:
Cause depcruiser to emit errors:
These work. It's there. I promise.
Possible Solution
Per the docs, it sounds like module resolution is done via webpack? If it was possible to do using Node.js' own facilities, I imagine this would work. It could be that webpack just doesn't understand the
exportsfield inyargs. Which, for reference, is this:I'm thinking that this is actually two separate problems.
For the
yargs/yargs, maybe one of:For
yargs/helpers:The module resolution interprets Yargs'
"type": "module"to mean that any.jsfile referenced in its exports is an ES module, regardless of the conditional used. I don't know whether that's true off the top of my head, but it's plausible. But.... thehelpersfolder also contains its ownpackage.jsonwith"type": "commonjs", which evidently overrides this behavior. This is the sort of thing you don't do unless you have a reason. I don't know the reason, but my wild-ass guess is "backwards compatibility with older versions of Node.js".Steps to Reproduce (for bugs)
See the "Current Behavior" section. Run dependency cruiser using a freshly-init'd config on a file containing the example code.
Context
Running dep cruiser on Node.js project which outputs CJS, written in TS.
Your Environment