Bugfix: fix circular dependency loop in dev

#3466

.changeset/curvy-nails-rest.md

@@ -0,0 +1,5 @@
+---
+'snowpack': patch
+---
+
+Bugfix: dev server hanging on circular dependencies

snowpack/src/sources/local.ts

@@ -33,12 +33,12 @@ import {installPackages} from './local-install';
 
 const CURRENT_META_FILE_CONTENTS = `.snowpack cache - Do not edit this directory!
 
-The ".snowpack" cache directory is fully managed for you by Snowpack. 
+The ".snowpack" cache directory is fully managed for you by Snowpack.
 Manual changes that you make to the files inside could break things.
 
 Commit this directory to source control to speed up cold starts.
 
-Found an issue? You can always delete the ".snowpack" 
+Found an issue? You can always delete the ".snowpack"
 directory and Snowpack will recreate it on next run.
 
 [.meta.version=2]`;
@@ -112,6 +112,7 @@ export class PackageSourceLocal implements PackageSource {
   cacheDirectory: string;
   packageSourceDirectory: string;
   memoizedResolve: Record<string, string> = {};
+  memoizedImportMap: Record<string, ImportMap> = {};
   allPackageImports: Record<string, PackageImportData> = {};
   allSymlinkImports: Record<string, string> = {};
   allKnownSpecs = new Set<string>();
@@ -425,7 +426,7 @@ export class PackageSourceLocal implements PackageSource {
   }
 
   private async buildPackageImport(spec: string, _source?: string, logLine = false, depth = 0) {
-    const {config, memoizedResolve, allKnownSpecs, allPackageImports} = this;
+    const {config, memoizedResolve, memoizedImportMap, allKnownSpecs, allPackageImports} = this;
     const source = _source || this.packageSourceDirectory;
     const aliasEntry = findMatchingAliasEntry(config, spec);
     if (aliasEntry && aliasEntry.type === 'package') {
@@ -494,13 +495,10 @@ export class PackageSourceLocal implements PackageSource {
       const lineBullet = colors.dim(depth === 0 ? '+' : '└──'.padStart(depth * 2 + 1, ' '));
       let packageFormatted = spec + colors.dim('@' + packageVersion);
       const existingImportMapLoc = path.join(installDest, 'import-map.json');
-      const importMapHandle = await fs.open(existingImportMapLoc, 'r+').catch(() => null);
-
-      let existingImportMap: ImportMap | null = null;
-      if (importMapHandle) {
-        const importMapData = await importMapHandle.readFile('utf-8');
-        existingImportMap = importMapData ? JSON.parse(importMapData) : null;
-        await importMapHandle.close();
+      let existingImportMap: ImportMap | undefined = memoizedImportMap[packageName];
+      if (!existingImportMap && existsSync(existingImportMapLoc)) {
+        existingImportMap = JSON.parse(await fs.readFile(existingImportMapLoc, 'utf8'));
+        memoizedImportMap[packageName] = existingImportMap as ImportMap;
       }
 
       // Kick off a build, if needed.
@@ -624,20 +622,23 @@ export class PackageSourceLocal implements PackageSource {
       const dependencyFileLoc = path.join(installDest, importMap.imports[spec]);
       const loadedFile = await fs.readFile(dependencyFileLoc!);
       if (isJavaScript(dependencyFileLoc)) {
-        const packageImports = new Set<string>();
+        const newPackageImports = new Set<string>();
         const code = loadedFile.toString('utf8');
         for (const imp of await scanCodeImportsExports(code)) {
-          const spec = code.substring(imp.s, imp.e).replace(/(\/|\\)+$/, ''); // remove trailing slash from end of specifier (easier for Node to resolve)
-          if (isRemoteUrl(spec)) {
-            continue;
+          const scannedImport = code.substring(imp.s, imp.e).replace(/(\/|\\)+$/, ''); // remove trailing slash from end of specifier (easier for Node to resolve)
+          if (isRemoteUrl(scannedImport)) {
+            continue; // ignore remote files
           }
-          if (isPathImport(spec)) {
+          if (isPathImport(scannedImport)) {
             continue;
           }
-          packageImports.add(spec);
+          if (memoizedImportMap[scannedImport]) {
+            continue; // if we’ve already installed this, then don’t reinstall
+          }
+          newPackageImports.add(scannedImport);
         }
 
-        for (const packageImport of packageImports) {
+        for (const packageImport of newPackageImports) {
           await this.buildPackageImport(packageImport, entrypoint, logLine, depth + 1);
         }
       }

test/snowpack/package/@nivo/index.test.js

@@ -0,0 +1,35 @@
+const dedent = require('dedent');
+const {testFixture, testRuntimeFixture} = require('../../../fixture-utils');
+const {fmtjson} = require('../../../test-utils');
+
+const pkg = {
+  'index.js': dedent`
+    import {NetworkCanvas} from '@nivo/network';
+  `,
+  'package.json': fmtjson({
+    dependencies: {
+      '@nivo/core': '^0.72.0',
+      '@nivo/network': '^0.72.0',
+    },
+  }),
+};
+
+/**
+ * Fixes #3466
+ * Main thing we’re testing for here is that dev server doesn’t hang on circular deps
+ * Though this test is slow, it’s important to test on real npm packages and not mocked ones
+ * as symlink behavior is really different here
+ */
+describe('@nivo/core', () => {
+  it('dev', async () => {
+    const server = await testRuntimeFixture(pkg);
+    const js = (await server.loadUrl('/index.js')).contents.toString('utf8');
+    expect(js).toBeTruthy(); // if this returned some response,
+    await server.cleanup(); // clean up
+  }, 60000); // wait an extra long time
+
+  it('build', async () => {
+    const result = await testFixture(pkg);
+    expect(result['index.js']).toBeTruthy();
+  });
+});

test/test-utils.js

@@ -156,3 +156,9 @@ function stripLockfile(output) {
   return stripWhitespace(stripUrlHash(output));
 }
 exports.stripLockfile = stripLockfile;
+
+/** Format JSON */
+function fmtjson(json) {
+  return JSON.stringify(json, undefined, 2);
+}
+exports.fmtjson = fmtjson;