sverweij / dependency-cruiser

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

Issue: numberOfDependentsLessThan does not calculate dependents properly when index.ts is used inbetween #896

Closed Sp0tlight closed 7 months ago

Sp0tlight commented 8 months ago

Expected Behavior

numberOfDependentsLessThan should also calculate implicit dependencies when an index.ts is used to aggregate export of folders.

Current Behavior

numberOfDependentsLessThan do not take external dependencies through index.ts aggregation into consideration, only the 1 very direct relation to the index.ts is considered.

Steps to Reproduce

Fairly simple setup:

libs/dep-cruiser-test/parent-components -> Embeding child from shared folder
libs/dep-cruiser-test/shared/files-to-import-by-index-ts -> Childs that will be exported by a index.ts in the given folder and imported from the parents
libs/dep-cruiser-test/shared/files-to-directly-import -> Childs that will be directly imported from the parents

with the following rule:

{
  name: 'shared-components-should-be-properly-reused',
  severity: 'error',
  from: {
    path: '^libs/dep-cruiser-test',
    pathNot: '^libs/dep-cruiser-test/shared',
  },
  module: {
    path: '^libs/dep-cruiser-test/shared',
    pathNot: 'index\\.ts$',
    numberOfDependentsLessThan: 2,
  },
}

Leads to violation of the given rule for the childs that are exported by the index.ts

image

Context

Any workarounds or other solutions are highly welcome.

Your Environment

sverweij commented 8 months ago

Hi @Sp0tlight thanks for sharing this. The numberOfDependentsLessThan and numberOfDependentsMoreThan restrictions work on the direct dependents of a module. As the parent-components don't directly depend on indexTsChild*.tsx (but instead via another module) they don't count towards the number of direct dependents.

Sp0tlight commented 8 months ago

Hi @sverweij , yep I figured this out later on as I took a deeper look into the codebase. As I still have the requirement (and probably all other mono-repos) that dep-cruiser is ignoring re-exports, as they aren't a real dependency I also took the chance to modify the logic a bit locally to achieve it.

E. g. here I have a graph where both is shown: index.ts dependencies, as well as the indirect ones. Also the type of dependencies are distinguishable, so you can pretty easily style it differently to get the full picture or also adapt the rules clauses to your needs (new dependency type: BypassingIndexFile)

{
  criteria: { isIndexFileDependency: true },
  attributes: {
    color: '#A8A8A877',
    weight: 0.2,
    penwidth: 0.5,
    arrowsize: 0.4,
    style: 'dashed',
  },
},
{
  criteria: { resolve: 'index.ts' },
  attributes: {
    color: '#A8A8A877',
    weight: 0.2,
    penwidth: 0.5,
    arrowsize: 0.4,
    style: 'dashed',
  },
},
{
  criteria: { isBypassingIndexFile: true },  // the indirect dependencies from re-export chains
  attributes: {
    color: '#343434D8',
    penwidth: 0.7,
    edgetooltip: 'indirect dependency (bypassing index file)',
    style: 'dashed',
  },
}
image

Is this something that you want to be part of dep-cruiser? So far the code is really dirty and doesn't fit good into the current dep-cruiser arch, as I just needed some q&d solution. But if it's interesting for you I could create a small PR and maybe you could take it from there to align it better with your ideas/requirements for the arch.

sverweij commented 8 months ago

Hi @Sp0tlight I see the need, but it does cross a principle I set long time ago to keep the focus on dependencies only and do that well. Deeper static analysis didn't fit into that. I'm ok with revising that decision, but am a bit afraid it'll mean an explosion of complexity - so I'm curious about the solution you came up with (regardless whether or not it fits within dependency-cruiser's current 'architecture' or its lack of cleanliness) -

E.g. some of the lions & bears I see on the road:

// barrel.ts
import { thing, type otherthing } from './other-local-thing';
import * as haystack from './more-local-shopping';
export * from 'something-from-npm';
export { usefulFunction } from 'other-thing-from-npm';
export * from './something-local';
export * from '../somewhere/else';
export type { localType } from './something-local';
export const renamedThing = thing;
export const baalHooi = haystack;

// something-local.ts
export * from './more-local-one'
export * from './more-local-two'

// ...

// some consumer
import { needle } from './barrel'; // how to find 'needle'

// some other consumer
import { baalHooi } from './barrel'; // baalHooi is actually a re-assignment of haystack, so ...?

state-machine-2024-01-08T20 53 44 122Z

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.

Eux86 commented 7 months ago

I have been looking for something similar to what has been shown in this thread for a while. Having the option to show the "actually" dependency you show in your example, can be very valuable when importing from a folder with a barrel file and lots of dependencies below it. It's true that strictly talking about JS modules, in your previous example there is a direct dependency between things.js and index.ts, but when I want to analyse the architecture of how each "piece of the puzzle" depends on another piece, the dependency to some-feature.ts has much more value then the one to the index.ts file.

Is there any plan to have this option?

(by the way thanks for the amazing tool)