Bugfix: fix circular dependency loop in dev #3562

drwpow · 2021-07-08T21:18:16Z

Changes

Fixes #3466. If an npm dependency depends on another, we were trying to install both over and over again without checking. This PR memoizes resolved npm packages so each is only hit once.

This should also improve the dev server cold start time by a little bit, as packages are better-memoized now.

Testing

Test added

Docs

Internal change; no docs needed

changeset-bot · 2021-07-08T21:18:18Z

🦋 Changeset detected

Latest commit: 8137901

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
snowpack	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

vercel · 2021-07-08T21:18:19Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/pikapkg/snowpack/6ENtfdTXMyfVGESwUAfaao8GqnPb
✅ Preview: https://snowpack-git-fix-circular-dev-install-pikapkg.vercel.app

snowpack/src/sources/local.ts

drwpow · 2021-07-08T21:21:02Z

snowpack/src/sources/local.ts

-          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)


minor change: don’t shadow spec in parent scope; it’s confusing.

drwpow · 2021-07-08T21:23:49Z

snowpack/src/sources/local.ts

            continue;
          }
-          packageImports.add(spec);
+          const [scannedPackageName] = parsePackageImportSpecifier(scannedImport);
+          if (scannedPackageName && memoizedImportMap[scannedPackageName]) {


This is the actual fix here: using the in-memory import maps to prevent reinstalling duplicate packages (import map means install happened successfully). Otherwise, we would end up stuck in an infinite loop installing circular npm deps.

matthewp · 2021-07-09T15:08:11Z

snowpack/src/sources/local.ts

-        await importMapHandle.close();
+      let existingImportMap: ImportMap | undefined = memoizedImportMap[packageName];
+      if (!existingImportMap && existsSync(existingImportMapLoc)) {
+        existingImportMap = JSON.parse(await fs.readFile(existingImportMapLoc, 'utf8'));


Using existsSync and then readFile still scares me as the previous bug was that the file existed but was not open for writing (because currently being written) which caused it to throw at this line. Even if logic prevents the race we should still use fs.open here as that guarantees both that the file exists and that it's finished being written to.

Oh you have a good point. Will change back, but probably keep the in-memory memoization for already-read files

hklsiteimprove · 2021-08-13T07:37:16Z

This PR might fix the following bug as well: #3188
@drwpow @matthewp do you believe this could be solved? and if so, is there anything I can do to help or push it through? 👍

hklsiteimprove · 2021-08-18T13:53:13Z

This PR might fix the following bug as well: #3188
@drwpow @matthewp do you believe this could be solved? and if so, is there anything I can do to help or push it through?

I looked at it and it does seem to solve the bug - at least it does not keep re-resolving the same packages again and again.
I was unable to verify it completely with the local linking, as it had some complications with other packages, but I believe it is good.

Who can help move it forward? :)

drwpow · 2021-08-24T19:49:01Z

@hklsiteimprove sorry about this! It fell off my plate. Updated the PR and this should go out in the next release after it merges

drwpow · 2021-08-24T20:37:11Z

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

+ * 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.skip('@nivo/core', () => {


As the comment said, added a test here which can be run manually. But skipping this in GitHub because for some reason this test keeps flaking in GitHub CI. But I’m able to run it on my machine in Node 12, 14, and 16 without fail every time. So this does fix the issue!

hklsiteimprove · 2021-08-25T06:28:39Z

@drwpow This PR fixed the circular resolving really nicely.
It did, however, break the web test runner import for the dev server

Error while running tests:
Error: Cannot find module ''../../../../../__web-dev-server__web-socket.js'' from '/opt/app/node_modules/@web/test-runner-commands/browser/commands.mjs'
    at Function.resolveSync [as sync] (/opt/app/node_modules/resolve/lib/sync.js:102:15)
    at Object.resolveEntrypoint (/opt/app/node_modules/esinstall/lib/entrypoints.js:149:59)
    at PackageSourceLocal.buildPackageImport (/opt/app/node_modules/snowpack/lib/cjs/sources/local.js:424:40)
    at async /opt/app/node_modules/snowpack/lib/cjs/sources/local.js:606:21
    at async PackageSourceLocal.buildPackageImport (/opt/app/node_modules/snowpack/lib/cjs/sources/local.js:454:30)
    at async /opt/app/node_modules/snowpack/lib/cjs/sources/local.js:606:21
    at async PackageSourceLocal.buildPackageImport (/opt/app/node_modules/snowpack/lib/cjs/sources/local.js:454:30)
    at async PackageSourceLocal.prepare (/opt/app/node_modules/snowpack/lib/cjs/sources/local.js:227:13)
    at async Object.startServer (/opt/app/node_modules/snowpack/lib/cjs/commands/dev.js:240:9)
    at async Object.serverStart (/opt/app/node_modules/@snowpack/web-test-runner-plugin/plugin.js:33:16) {
  code: 'MODULE_NOT_FOUND'
}

I can apply a fix inside the resolving of newPackages where I simply opt out of resolving this package, and it works. But it seems like the wrong place, and it should have been handled somewhere else. Any ideas?

drwpow commented Jul 8, 2021

View reviewed changes

snowpack/src/sources/local.ts Show resolved Hide resolved

drwpow commented Jul 8, 2021

View reviewed changes

drwpow force-pushed the fix-circular-dev-install branch from 0cedb3b to 31a8b52 Compare July 8, 2021 21:23

vercel bot deployed to Preview July 8, 2021 21:23 View deployment

drwpow commented Jul 8, 2021

View reviewed changes

drwpow force-pushed the fix-circular-dev-install branch from 31a8b52 to 5968922 Compare July 8, 2021 21:27

vercel bot deployed to Preview July 8, 2021 21:27 View deployment

matthewp reviewed Jul 9, 2021

View reviewed changes

Bugfix: fix circular dependency loop in dev

a0e39df

drwpow force-pushed the fix-circular-dev-install branch from 5968922 to a0e39df Compare August 23, 2021 17:44

vercel bot deployed to Preview August 23, 2021 17:44 View deployment

vercel bot deployed to Preview August 23, 2021 17:48 View deployment

drwpow force-pushed the fix-circular-dev-install branch from b279c67 to 6b8d9aa Compare August 23, 2021 17:58

vercel bot deployed to Preview August 23, 2021 17:58 View deployment

hklsiteimprove approved these changes Aug 24, 2021

View reviewed changes

drwpow force-pushed the fix-circular-dev-install branch from 6b8d9aa to 480ff48 Compare August 24, 2021 19:43

vercel bot deployed to Preview August 24, 2021 19:43 View deployment

drwpow force-pushed the fix-circular-dev-install branch from 480ff48 to 3c8cab7 Compare August 24, 2021 19:44

vercel bot deployed to Preview August 24, 2021 19:44 View deployment

drwpow force-pushed the fix-circular-dev-install branch from 3c8cab7 to db0851e Compare August 24, 2021 19:57

vercel bot deployed to Preview August 24, 2021 19:58 View deployment

drwpow force-pushed the fix-circular-dev-install branch from db0851e to ef7a2b9 Compare August 24, 2021 19:59

vercel bot deployed to Preview August 24, 2021 19:59 View deployment

drwpow force-pushed the fix-circular-dev-install branch from ef7a2b9 to ebfc585 Compare August 24, 2021 20:02

vercel bot deployed to Preview August 24, 2021 20:02 View deployment

drwpow force-pushed the fix-circular-dev-install branch from ebfc585 to 3974fb4 Compare August 24, 2021 20:18

vercel bot deployed to Preview August 24, 2021 20:18 View deployment

drwpow force-pushed the fix-circular-dev-install branch from 3974fb4 to f72f346 Compare August 24, 2021 20:22

vercel bot deployed to Preview August 24, 2021 20:23 View deployment

drwpow force-pushed the fix-circular-dev-install branch from f72f346 to 06ca409 Compare August 24, 2021 20:25

vercel bot deployed to Preview August 24, 2021 20:25 View deployment

PR feedback

8137901

drwpow force-pushed the fix-circular-dev-install branch from 06ca409 to 8137901 Compare August 24, 2021 20:25

vercel bot deployed to Preview August 24, 2021 20:25 View deployment

drwpow commented Aug 24, 2021

View reviewed changes

drwpow merged commit d72dc7d into main Aug 24, 2021

drwpow deleted the fix-circular-dev-install branch August 24, 2021 20:39

canderis mentioned this pull request Aug 25, 2021

🐛 BUG: Circular dependency fix broke starting up with some packages #3682

Open

1 task

matthewp mentioned this pull request Aug 26, 2021

Fixes dynamic import regression #3684

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix: fix circular dependency loop in dev #3562

Bugfix: fix circular dependency loop in dev #3562

drwpow commented Jul 8, 2021 •

edited

changeset-bot bot commented Jul 8, 2021 •

edited

vercel bot commented Jul 8, 2021 •

edited

drwpow Jul 8, 2021

drwpow Jul 8, 2021 •

edited

matthewp Jul 9, 2021

drwpow Jul 9, 2021

hklsiteimprove commented Aug 13, 2021

hklsiteimprove commented Aug 18, 2021

drwpow commented Aug 24, 2021

drwpow Aug 24, 2021

hklsiteimprove commented Aug 25, 2021 •

edited

Bugfix: fix circular dependency loop in dev #3562

Bugfix: fix circular dependency loop in dev #3562

Conversation

drwpow commented Jul 8, 2021 • edited

Changes

Testing

Docs

changeset-bot bot commented Jul 8, 2021 • edited

🦋 Changeset detected

vercel bot commented Jul 8, 2021 • edited

drwpow Jul 8, 2021

Choose a reason for hiding this comment

drwpow Jul 8, 2021 • edited

Choose a reason for hiding this comment

matthewp Jul 9, 2021

Choose a reason for hiding this comment

drwpow Jul 9, 2021

Choose a reason for hiding this comment

hklsiteimprove commented Aug 13, 2021

hklsiteimprove commented Aug 18, 2021

drwpow commented Aug 24, 2021

drwpow Aug 24, 2021

Choose a reason for hiding this comment

hklsiteimprove commented Aug 25, 2021 • edited

drwpow commented Jul 8, 2021 •

edited

changeset-bot bot commented Jul 8, 2021 •

edited

vercel bot commented Jul 8, 2021 •

edited

drwpow Jul 8, 2021 •

edited

hklsiteimprove commented Aug 25, 2021 •

edited