feat: warn about cyclic dependencies (#2162) (#3537)

.changeset/kind-beds-kneel.md

@@ -0,0 +1,5 @@
+---
+"@pnpm/sort-packages": minor
+---
+
+Expose raw graph-sequencer result through sequenceGraph

.changeset/seven-donuts-yawn.md

@@ -0,0 +1,6 @@
+---
+"@pnpm/plugin-commands-installation": minor
+"@pnpm-private/typings": minor
+---
+
+Warn about cyclic dependencies on install

packages/plugin-commands-installation/src/installDeps.ts

@@ -21,6 +21,8 @@ import getSaveType from './getSaveType'
 import recursive, { createMatcher, matchDependencies } from './recursive'
 import updateToLatestSpecsFromManifest, { createLatestSpecs } from './updateToLatestSpecsFromManifest'
 import { createWorkspaceSpecs, updateToWorkspacePackagesFromManifest } from './updateWorkspaceDependencies'
+import logger from '@pnpm/logger'
+import { sequenceGraph } from '@pnpm/sort-packages'
 
 const OVERWRITE_UPDATE_OPTIONS = {
   allowNew: true,
@@ -120,6 +122,18 @@ when running add/update with the --workspace option')
   if (opts.workspaceDir) {
     const selectedProjectsGraph = opts.selectedProjectsGraph ?? selectProjectByDir(allProjects, opts.dir)
     if (selectedProjectsGraph != null) {
+      const sequencedGraph = sequenceGraph(selectedProjectsGraph)
+      // Check and warn if there are cyclic dependencies
+      if (!sequencedGraph.safe) {
+        const cyclicDependenciesInfo = sequencedGraph.cycles.length > 0
+          ? `: ${sequencedGraph.cycles.map(deps => deps.join(', ')).join('; ')}`
+          : ''
+        logger.warn({
+          message: `There are cyclic dependencies${cyclicDependenciesInfo}`,
+          prefix: opts.workspaceDir,
+        })
+      }
+
       await recursive(allProjects,
         params,
         {

packages/plugin-commands-installation/test/warnCyclicDependencies.ts

@@ -0,0 +1,70 @@
+import { install } from '@pnpm/plugin-commands-installation'
+import { readProjects } from '@pnpm/filter-workspace-packages'
+import { preparePackages } from '@pnpm/prepare'
+import { DEFAULT_OPTS } from './utils'
+import logger from '@pnpm/logger'
+
+beforeEach(() => {
+  jest.spyOn(logger, 'warn')
+})
+
+afterEach(() => {
+  (logger.warn as jest.Mock).mockRestore()
+})
+
+test('should warn about cyclic dependencies', async () => {
+  preparePackages([
+    {
+      name: 'project-1',
+      version: '1.0.0',
+      dependencies: { 'project-2': 'workspace:*' },
+    },
+    {
+      name: 'project-2',
+      version: '2.0.0',
+      devDependencies: { 'project-1': 'workspace:*' },
+    },
+  ])
+
+  const { allProjects, selectedProjectsGraph } = await readProjects(process.cwd(), [])
+  await install.handler({
+    ...DEFAULT_OPTS,
+    allProjects,
+    dir: process.cwd(),
+    recursive: true,
+    selectedProjectsGraph,
+    workspaceDir: process.cwd(),
+  })
+
+  expect(logger.warn).toHaveBeenCalledTimes(1)
+  expect(logger.warn).toHaveBeenCalledWith({
+    message: expect.stringMatching(/^There are cyclic dependencies: /),
+    prefix: process.cwd(),
+  })
+})
+
+test('should not warn about cyclic dependencies if there are not', async () => {
+  preparePackages([
+    {
+      name: 'project-1',
+      version: '1.0.0',
+      dependencies: { 'project-2': 'workspace:*' },
+    },
+    {
+      name: 'project-2',
+      version: '2.0.0',
+    },
+  ])
+
+  const { allProjects, selectedProjectsGraph } = await readProjects(process.cwd(), [])
+  await install.handler({
+    ...DEFAULT_OPTS,
+    allProjects,
+    dir: process.cwd(),
+    recursive: true,
+    selectedProjectsGraph,
+    workspaceDir: process.cwd(),
+  })
+
+  expect(logger.warn).toHaveBeenCalledTimes(0)
+})

packages/sort-packages/src/index.ts

@@ -1,7 +1,8 @@
 import { ProjectsGraph } from '@pnpm/types'
 import graphSequencer from 'graph-sequencer'
+import type { Result as GraphSequencerResult } from 'graph-sequencer'
 
-export default function sortPackages (pkgGraph: ProjectsGraph): string[][] {
+export function sequenceGraph (pkgGraph: ProjectsGraph): GraphSequencerResult<string> {
   const keys = Object.keys(pkgGraph)
   const setOfKeys = new Set(keys)
   const graph = new Map(
@@ -60,9 +61,13 @@ export default function sortPackages (pkgGraph: ProjectsGraph): string[][] {
         setOfKeys.has(d))]
     )
   )
-  const graphSequencerResult = graphSequencer({
+  return graphSequencer({
     graph,
     groups: [keys],
   })
+}
+
+export default function sortPackages (pkgGraph: ProjectsGraph): string[][] {
+  const graphSequencerResult = sequenceGraph(pkgGraph)
   return graphSequencerResult.chunks
 }

typings/local.d.ts

@@ -84,8 +84,26 @@ declare module 'graceful-git' {
 }
 
 declare module 'graph-sequencer' {
-  const anything: any;
-  export = anything;
+  namespace graphSequencer {
+    type Graph<T> = Map<T, T[]>;
+    type Groups<T> = Array<T[]>;
+
+    interface Options<T> {
+      graph: Graph<T>;
+      groups: Groups<T>;
+    }
+
+    interface Result<T> {
+      safe: boolean;
+      chunks: Groups<T>;
+      cycles: Groups<T>;
+    }
+
+    type GraphSequencer = <T>(opts: Options<T>) => Result<T>;
+  }
+
+  const graphSequencer: graphSequencer.GraphSequencer;
+  export = graphSequencer;
 }
 
 declare module 'is-inner-link' {