Issue: no-circular does not follow dependencyTypesNot: ['type-only'] over entire cycle

[camillef]

I am using dependency-cruiser in a React app written in Typescript. I would like to exclude circular type-only references from being reported as errors by dependency-cruiser, so I have updated my "no-circular" rule to include dependencyTypesNot: ["type-only"], as follows:

    {
      name: "no-circular",
      severity: "error",
      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,
        dependencyTypesNot: ["type-only"],
      },
    }

However, this still reports errors for both type-only circular dependencies, and if any part of the circular dependency path is not type-only.

Expected Behavior

"dependencyTypesNot": ["type-only"] applied to a "no-circular" rule should mean that circular dependencies involving type-only imports do not trigger an error

Current Behavior

dependencyTypesNot: ["type-only"] has no effect when circular: true

Possible Solution

Not sure. I wondered if viaNot might also be able to take into account the dependency type.

Steps to Reproduce (for bugs)

1. Create a parent.ts file and a child.ts file as follows

// parent.ts
import type { Child } from "./child";

export interface Parent {
  children: Child[];
}

// child.ts
import type { Parent } from "./parent";

export interface Child {
  parent: Parent;
}

2. Install dependency-cruiser and add the following config to dependency-cruiser.js

    {
      name: "no-circular",
      severity: "error",
      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,
        dependencyTypesNot: ["type-only"],
      },
    }

3. Run depcruise --config .dependency-cruiser.js

Context

My app uses Redux for state management, and redux-toolkit listener middleware for certain behaviours. The redux-toolkit docs suggest keeping the store and its types in a store.ts file:

// store.ts
import { configureStore } from '@reduxjs/toolkit'
import { listenerMiddleware } from './listenerMiddleware'
// ...

export const store = configureStore({
  reducer: { todosReducer },

  middleware: (getDefaultMiddleware) =>
    getDefaultMiddleware().prepend(listenerMiddleware.middleware),
})

export type RootState = ReturnType<typeof store.getState>
export type AppDispatch = typeof store.dispatch 

and specifically recommend creating the middleware instance in a separate file from the actual configureStore() call, along with various helper types that depend on the RootState:

// listenerMiddleware.ts
import { createListenerMiddleware, addListener } from '@reduxjs/toolkit'
import type { TypedStartListening, TypedAddListener } from '@reduxjs/toolkit'

import type { RootState, AppDispatch } from './store'

export const listenerMiddleware = createListenerMiddleware()

export type AppStartListening = TypedStartListening<RootState, AppDispatch>

export const startAppListening =
  listenerMiddleware.startListening as AppStartListening

export const addAppListener = addListener as TypedAddListener<
  RootState,
  AppDispatch
>

Since the middleware must be added to the store when it is configured, the ./store file includes an import from ./listenerMiddleware. But the ./listenerMiddleware includes a type import from the ./store.

The only way I have around this, for now, is to exclude the ./store file with a viaNot.

Your Environment

• Version used: 12.0.1
• Node version: 16.10.0
• Operating System and version: MacOs 12.5.1

[sverweij]

Hi @camillef thank you for raising this issue - I'm looking into it (using your excellent reproduction steps as a guide).

[sverweij]

hi @camillef - I've made a reproduction sample for the issue in the dependency-cruiser repro repo. There's a few things going on

The type-only dependency-type only identifies explicitly type only imports and exports as introduced in typescript 3.8. In the linked reproduction sample you can see that this indeed works as expected for the straightforward parent - child cycle and even for longer cycles that are entirely made up of explicit type-only dependencies.

I see you already edited the original issue to include these explicit type's - so I guess you found this out yourself already. You might also have found, that even with that there's an (incorrect, I think) behaviour in dependency-cruiser's cycle detection algorithm in that when only one dependency in a cycle is not

flowchart LR
  parent -->|type-only| child
  child -->|type-only| grand-child
  grand-child --> parent

Loading

With the rule as defined in the issue, in the graph above parent -> child -> grand-child -> parent and the cycle starting on child will go unreported properly. However, the cycle starting on grand-child will still be flagged as violating the rule.

That bug will need to be ironed out of the cycle flagging code. I have ideas how to do that, but I'll need a patch of spare time to work on that (no promises, but Christmas holidays are coming up ...).

Options in the mean time:

• the viaNot attribute you already mention in the Context section
• set the tsPreCompilationDeps in .dependency-cruiser.js to false.
  That way dependency-cruiser will ignore all dependencies that only exist before transpiration from TypeScript to JavaScript meaning all type-only dependencies (both implicit and explicit) + unused dependencies. Downsides:
  • If you want to set up rules for type-only dependencies apart from the cycles, or simply show them in graphical output - these won't be detected or shown anymore.
  • It's a tad slower than with tsPreComilationDeps switched to true.
• set the tsPreCompilationDeps in .dependency-cruiser.js to "specify".
  I thought this option would be useful as well for a workaround, but it'd likely only solve a subset of your troubles - if any.
  With tsPreCompilationDeps on "specify" it becomes possible to use the preCompilationOnly attribute in rules. This attribute is true for all dependencies that only exist before compilation from TypeScript to JavaScript. That includes both implicit and explicit type-only dependencies (see below for a sample rule). So if the cycle(s) consists of only type-only dependencies (regardless whether they're explicitly declared as such) - they'll be ignored correctly. If there's a 'normal' dependency in the cycle it's back to square one, though. Another drawback is that it's it's again a tad slower than tsPreCompilationDeps switched to false.

sample rule with the preCompilationOnly attribute

// ...
  {
      name: "no-circular",
      severity: "error",
      from: {},
      to: {
        circular: true,
        preCompilationOnly: false,
      },
    },
// ...

[camillef]

Hi @sverweij,

Thank you for your thorough investigation - you're exactly correct that my reproduction was missing type indicators. Good to know that it's actually behaving correctly for explicit-type-only circular dependencies.

I think I'll stick with the viaNot exception for now and await a bug fix, as I'd like to retain as much dependency checking as possible (including other tsPreCompilationDeps) in the meantime.

[alvaro-cuesta]

@sverweij I'd be interested in working on this too since it's also affecting me (similar use case to @camillef's but for Remix loaders).

If I understand correctly, the issue will be fixed if the parent->child and child->grand-child paths are not followed in the circular dependency checker, right? That should break the cycle.

[sverweij]

Hi @alvaro-cuesta thanks for your interest in picking this up! There were quite a few things that needed doing to enable this feature - and t.b.h. I didn't oversee exactly which ones.

• dependency-cruiser's data model needed to be expanded so the list of via's in a cycle not only contained the name, but also the dependency type (a breaking change for the API).
• in order to do that the cycle detection algorithm needed to be tweaked
• the possibilities of the ruleset needed to be expanded to (1) express things that went over an entire cycle (2) that is consistent with how it's done elsewhere in rule sets.
• to ensure backwards compatibility for the regular (non-API) use, dependency-cruiser needed to grow some features on its cache and known violations mechanism
• reporters using this information (which is most of them) needed to be adapted as well.

Fixed: as of [email protected]

Anyway - in [email protected] it's now possible to place restrictions on cycles that follow all "via's" in the cycle. A rule way that works for the original use case of this issue:

  // ...

  // log an error for all circular dependencies which consist of only non-`type-only`
  // dependencies, so e.g.
  // violation : a -import-> b -import->           c -import-> a 
  // OK        : a -import-> b -import,type-only-> c -import-> a
  {
    name: 'no-circular-at-runtime',
    severity: 'error',
    from: {
    },
    to: {
      circular: true,
      viaOnly: {
        dependencyTypesNot: ['type-only']
      }
    }
  },

  // ...

Documentation: rules-reference.md#circular.

A similar exercise needs to be done for reachability rules, which should address #763.

[sverweij]

@camillef, @alvaro-cuesta (and all the people who upvoted this issue) [email protected] which fixes this issue has just been released. Do let me know if there's questions or additional issues!