Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #31240: Adding a recursive addPackagePath function in webpack-config #31264

Merged
merged 21 commits into from Feb 6, 2022

Conversation

neeraj3029
Copy link
Contributor

@neeraj3029 neeraj3029 commented Nov 10, 2021

Fixes: #31240
Closes: #32324
Adding a try-catch block to handle situations when packages are found at relative path in getPackagePath function. This is likely to occur when using preact instead of react-dom, as scheduler package will not be found wrt react-dom

Bug

  • Related issues linked using fixes #31240

@neeraj3029
Copy link
Contributor Author

neeraj3029 commented Nov 10, 2021

Hey @ijjk @timneutkens @huozhi , it would be great to have your review here.
This change will be an immediate unblocker to preact users like me :D

@blaketastic2
Copy link

I was able to roll back to v12.0.3 to get this to work, but can confirm that > v12.0.4 do not work.

sokra
sokra previously requested changes Nov 18, 2021
Copy link
Member

@sokra sokra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are bringing up a good point. We might be a bit more smart when creating these topLevelFrameworkPaths.

Instead of just assuming the scheduler is a dependency of react-dom, we should read the dependencies from package.json.

So how about re-writing the code to:

const topLevelFrameworkPaths = new Set()

for(const package of ["react", "react-dom", "next"]) {
  addPackagePath(name, dir)
}

and in addPackagePath recursively visiting the dependencies listed in package.json:

const packageJsonPath = require.resolve(`${name}/package.json`, {
  paths: [relativeToPath],
})

const directory = path.join(packageJsonPath, '../')
topLevelFrameworkPaths.add(directory)

const dependencies = require(packageJsonPath).dependencies
if(dependencies) {
  for(const name of Object.keys(dependencies)) {
    addPackagePath(name, directory)
  }
}

and adding a try-catch too...

@neeraj3029
Copy link
Contributor Author

neeraj3029 commented Nov 19, 2021

Hi @sokra @huozhi, updated the code. Now adding dep paths recursively. Changed the PR title accordingly.

@neeraj3029 neeraj3029 changed the title adding try-catch block to getPackagePath function Fixes #31240: Adding a recursive addPackagePath function in webpack-config Nov 19, 2021
@ijjk

This comment has been minimized.

@ijjk

This comment has been minimized.

@ijjk

This comment has been minimized.

@ijjk

This comment has been minimized.

@elliottsj
Copy link
Contributor

@neeraj3029 Looks like tests are failing due to these errors:

https://github.com/vercel/next.js/runs/4324606853?check_suite_focus=true

FAIL test/integration/production-build-dir/test/index.test.js
  Production Custom Build Directory
    With basic usage
      ✕ should render the page (3247 ms)

  ● Production Custom Build Directory › With basic usage › should render the page

    expect(received).toBe(expected) // Object.is equality

    Expected: ""
    Received: "Dependency '@babel/helper-validator-identifier' was not found relative to /home/runner/work/next.js/next.js/node_modules/@babel/highlight/. Optimized chunk splitting will be unavailable for '@babel/helper-validator-identifier'.
    Dependency 'get-intrinsic' was not found relative to /home/runner/work/next.js/next.js/node_modules/call-bind/. Optimized chunk splitting will be unavailable for 'get-intrinsic'.

Testing locally, the underlying (swallowed) error is likely this:

> require.resolve('@babel/helper-validator-identifier/package.json')
Uncaught:
Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: Package subpath './package.json' is not defined by "exports" in /Users/spencerelliott/dev/elliottsj/next.js/node_modules/@babel/helper-validator-identifier/package.json
    at throwExportsNotFound (internal/modules/esm/resolve.js:299:9)
    at packageExportsResolve (internal/modules/esm/resolve.js:522:3)
    at resolveExports (internal/modules/cjs/loader.js:449:36)
    at Function.Module._findPath (internal/modules/cjs/loader.js:489:31)
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:875:27)
    at Function.resolve (internal/modules/cjs/helpers.js:98:19)
    at REPL17:1:9
    at Script.runInThisContext (vm.js:134:12)
    at REPLServer.defaultEval (repl.js:488:29)
    at bound (domain.js:416:15) {
  code: 'ERR_PACKAGE_PATH_NOT_EXPORTED'

The require.resolve(`${packageName}/package.json`) expression relies on the /package.json subpath existing, so this seems to be a flawed assumption. I'm not sure if Node.js has a more canonical way to resolve path of the folder of a package, without the main module. Perhaps https://github.com/stefanpenner/resolve-package-path

@neeraj3029
Copy link
Contributor Author

Hey @ijjk @sokra, I've updated the code!

@ijjk

This comment has been minimized.

@ijjk

This comment has been minimized.

@OmerWow
Copy link

OmerWow commented Jan 22, 2022

Is there any update regarding this PR?

Preact is not usable at all right now and it'll. be awesome if you can restore functionality

@ijjk

This comment was marked as outdated.

@ijjk

This comment has been minimized.

@ijjk ijjk dismissed a stale review via 9f4db56 February 6, 2022 21:59
@ijjk
Copy link
Member

ijjk commented Feb 6, 2022

Stats from current PR

Default Build (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary neeraj3029/next.js neeraj3029try-catch-getPackagePath Change
buildDuration 18s 17.9s -101ms
buildDurationCached 7s 7s ⚠️ +24ms
nodeModulesSize 359 MB 359 MB ⚠️ +613 B
Page Load Tests Overall decrease ⚠️
vercel/next.js canary neeraj3029/next.js neeraj3029try-catch-getPackagePath Change
/ failed reqs 0 0
/ total time (seconds) 3.799 3.763 -0.04
/ avg req/sec 657.99 664.28 +6.29
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 1.549 1.57 ⚠️ +0.02
/error-in-render avg req/sec 1613.93 1592.45 ⚠️ -21.48
Client Bundles (main, webpack, commons) Overall decrease ✓
vercel/next.js canary neeraj3029/next.js neeraj3029try-catch-getPackagePath Change
450.HASH.js gzip 179 B 179 B
framework-HASH.js gzip 42.2 kB 42 kB -195 B
main-HASH.js gzip 27.4 kB 27.4 kB
webpack-HASH.js gzip 1.44 kB 1.44 kB
Overall change 71.2 kB 71 kB -195 B
Legacy Client Bundles (polyfills)
vercel/next.js canary neeraj3029/next.js neeraj3029try-catch-getPackagePath Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages Overall increase ⚠️
vercel/next.js canary neeraj3029/next.js neeraj3029try-catch-getPackagePath Change
_app-HASH.js gzip 1.36 kB 1.36 kB
_error-HASH.js gzip 194 B 194 B
amp-HASH.js gzip 312 B 312 B
css-HASH.js gzip 326 B 326 B
dynamic-HASH.js gzip 2.37 kB 2.57 kB ⚠️ +200 B
head-HASH.js gzip 350 B 350 B
hooks-HASH.js gzip 919 B 919 B
image-HASH.js gzip 4.94 kB 4.94 kB
index-HASH.js gzip 263 B 263 B
link-HASH.js gzip 2.2 kB 2.2 kB
routerDirect..HASH.js gzip 321 B 321 B
script-HASH.js gzip 383 B 383 B
withRouter-HASH.js gzip 318 B 318 B
85e02e95b279..7e3.css gzip 107 B 107 B
Overall change 14.4 kB 14.6 kB ⚠️ +200 B
Client Build Manifests
vercel/next.js canary neeraj3029/next.js neeraj3029try-catch-getPackagePath Change
_buildManifest.js gzip 460 B 460 B
Overall change 460 B 460 B
Rendered Page Sizes Overall decrease ✓
vercel/next.js canary neeraj3029/next.js neeraj3029try-catch-getPackagePath Change
index.html gzip 532 B 531 B -1 B
link.html gzip 546 B 545 B -1 B
withRouter.html gzip 526 B 526 B
Overall change 1.6 kB 1.6 kB -2 B

Diffs

Diff for _buildManifest.js
@@ -8,7 +8,7 @@ self.__BUILD_MANIFEST = {
     "static\u002Fchunks\u002Fpages\u002Fcss-97182c5b8324021a.js"
   ],
   "/dynamic": [
-    "static\u002Fchunks\u002Fpages\u002Fdynamic-9b7afaaa04b653ed.js"
+    "static\u002Fchunks\u002Fpages\u002Fdynamic-b49fdb7088c2e39d.js"
   ],
   "/head": ["static\u002Fchunks\u002Fpages\u002Fhead-7100d3b2a548f0e4.js"],
   "/hooks": ["static\u002Fchunks\u002Fpages\u002Fhooks-538d621a0e670391.js"],
Diff for dynamic-HASH.js
@@ -633,6 +633,79 @@
     ) {
       module.exports = __webpack_require__(638);
 
+      /***/
+    },
+
+    /***/ 8217: /***/ function(
+      __unused_webpack_module,
+      exports,
+      __webpack_require__
+    ) {
+      "use strict";
+      /** @license React vundefined
+       * use-subscription.production.min.js
+       *
+       * Copyright (c) Facebook, Inc. and its affiliates.
+       *
+       * This source code is licensed under the MIT license found in the
+       * LICENSE file in the root directory of this source tree.
+       */
+      var e = __webpack_require__(6086),
+        g = __webpack_require__(7294);
+      exports.useSubscription = function(a) {
+        var c = a.getCurrentValue,
+          d = a.subscribe,
+          b = g.useState(function() {
+            return { getCurrentValue: c, subscribe: d, value: c() };
+          });
+        a = b[0];
+        var f = b[1];
+        b = a.value;
+        if (a.getCurrentValue !== c || a.subscribe !== d)
+          (b = c()), f({ getCurrentValue: c, subscribe: d, value: b });
+        g.useDebugValue(b);
+        g.useEffect(
+          function() {
+            function b() {
+              if (!a) {
+                var b = c();
+                f(function(a) {
+                  return a.getCurrentValue !== c ||
+                    a.subscribe !== d ||
+                    a.value === b
+                    ? a
+                    : e({}, a, { value: b });
+                });
+              }
+            }
+            var a = !1,
+              h = d(b);
+            b();
+            return function() {
+              a = !0;
+              h();
+            };
+          },
+          [c, d]
+        );
+        return b;
+      };
+
+      /***/
+    },
+
+    /***/ 7161: /***/ function(
+      module,
+      __unused_webpack_exports,
+      __webpack_require__
+    ) {
+      "use strict";
+
+      if (true) {
+        module.exports = __webpack_require__(8217);
+      } else {
+      }
+
       /***/
     }
   },
Diff for framework-HASH.js
@@ -8688,76 +8688,6 @@
       } else {
       }
 
-      /***/
-    },
-
-    /***/ 8217: /***/ function(
-      __unused_webpack_module,
-      exports,
-      __webpack_require__
-    ) {
-      /** @license React vundefined
-       * use-subscription.production.min.js
-       *
-       * Copyright (c) Facebook, Inc. and its affiliates.
-       *
-       * This source code is licensed under the MIT license found in the
-       * LICENSE file in the root directory of this source tree.
-       */
-      var e = __webpack_require__(6086),
-        g = __webpack_require__(7294);
-      exports.useSubscription = function(a) {
-        var c = a.getCurrentValue,
-          d = a.subscribe,
-          b = g.useState(function() {
-            return { getCurrentValue: c, subscribe: d, value: c() };
-          });
-        a = b[0];
-        var f = b[1];
-        b = a.value;
-        if (a.getCurrentValue !== c || a.subscribe !== d)
-          (b = c()), f({ getCurrentValue: c, subscribe: d, value: b });
-        g.useDebugValue(b);
-        g.useEffect(
-          function() {
-            function b() {
-              if (!a) {
-                var b = c();
-                f(function(a) {
-                  return a.getCurrentValue !== c ||
-                    a.subscribe !== d ||
-                    a.value === b
-                    ? a
-                    : e({}, a, { value: b });
-                });
-              }
-            }
-            var a = !1,
-              h = d(b);
-            b();
-            return function() {
-              a = !0;
-              h();
-            };
-          },
-          [c, d]
-        );
-        return b;
-      };
-
-      /***/
-    },
-
-    /***/ 7161: /***/ function(
-      module,
-      __unused_webpack_exports,
-      __webpack_require__
-    ) {
-      if (true) {
-        module.exports = __webpack_require__(8217);
-      } else {
-      }
-
       /***/
     }
   }
Diff for index.html
@@ -15,7 +15,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/framework-535325b9235a831f.js"
+      src="/_next/static/chunks/framework-287ad9396a03b869.js"
       defer=""
     ></script>
     <script
Diff for link.html
@@ -15,7 +15,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/framework-535325b9235a831f.js"
+      src="/_next/static/chunks/framework-287ad9396a03b869.js"
       defer=""
     ></script>
     <script
Diff for withRouter.html
@@ -15,7 +15,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/framework-535325b9235a831f.js"
+      src="/_next/static/chunks/framework-287ad9396a03b869.js"
       defer=""
     ></script>
     <script

Default Build with SWC (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary neeraj3029/next.js neeraj3029try-catch-getPackagePath Change
buildDuration 21.9s 21.7s -204ms
buildDurationCached 7s 7.2s ⚠️ +124ms
nodeModulesSize 359 MB 359 MB ⚠️ +613 B
Page Load Tests Overall increase ✓
vercel/next.js canary neeraj3029/next.js neeraj3029try-catch-getPackagePath Change
/ failed reqs 0 0
/ total time (seconds) 3.767 3.712 -0.05
/ avg req/sec 663.61 673.47 +9.86
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 1.58 1.547 -0.03
/error-in-render avg req/sec 1582.62 1616.07 +33.45
Client Bundles (main, webpack, commons) Overall decrease ✓
vercel/next.js canary neeraj3029/next.js neeraj3029try-catch-getPackagePath Change
450.HASH.js gzip 179 B 179 B
framework-HASH.js gzip 42.3 kB 42.1 kB -190 B
main-HASH.js gzip 27.4 kB 27.4 kB
webpack-HASH.js gzip 1.44 kB 1.44 kB
Overall change 71.3 kB 71.2 kB -190 B
Legacy Client Bundles (polyfills)
vercel/next.js canary neeraj3029/next.js neeraj3029try-catch-getPackagePath Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages Overall increase ⚠️
vercel/next.js canary neeraj3029/next.js neeraj3029try-catch-getPackagePath Change
_app-HASH.js gzip 1.35 kB 1.35 kB
_error-HASH.js gzip 180 B 180 B
amp-HASH.js gzip 305 B 305 B
css-HASH.js gzip 321 B 321 B
dynamic-HASH.js gzip 2.36 kB 2.56 kB ⚠️ +193 B
head-HASH.js gzip 342 B 342 B
hooks-HASH.js gzip 911 B 911 B
image-HASH.js gzip 4.97 kB 4.97 kB
index-HASH.js gzip 256 B 256 B
link-HASH.js gzip 2.22 kB 2.22 kB
routerDirect..HASH.js gzip 314 B 314 B
script-HASH.js gzip 375 B 375 B
withRouter-HASH.js gzip 309 B 309 B
85e02e95b279..7e3.css gzip 107 B 107 B
Overall change 14.3 kB 14.5 kB ⚠️ +193 B
Client Build Manifests
vercel/next.js canary neeraj3029/next.js neeraj3029try-catch-getPackagePath Change
_buildManifest.js gzip 459 B 459 B
Overall change 459 B 459 B
Rendered Page Sizes Overall increase ⚠️
vercel/next.js canary neeraj3029/next.js neeraj3029try-catch-getPackagePath Change
index.html gzip 533 B 534 B ⚠️ +1 B
link.html gzip 546 B 547 B ⚠️ +1 B
withRouter.html gzip 528 B 528 B
Overall change 1.61 kB 1.61 kB ⚠️ +2 B

Diffs

Diff for _buildManifest.js
@@ -8,7 +8,7 @@ self.__BUILD_MANIFEST = {
     "static\u002Fchunks\u002Fpages\u002Fcss-97182c5b8324021a.js"
   ],
   "/dynamic": [
-    "static\u002Fchunks\u002Fpages\u002Fdynamic-9b7afaaa04b653ed.js"
+    "static\u002Fchunks\u002Fpages\u002Fdynamic-b49fdb7088c2e39d.js"
   ],
   "/head": ["static\u002Fchunks\u002Fpages\u002Fhead-7100d3b2a548f0e4.js"],
   "/hooks": ["static\u002Fchunks\u002Fpages\u002Fhooks-538d621a0e670391.js"],
Diff for dynamic-HASH.js
@@ -633,6 +633,79 @@
     ) {
       module.exports = __webpack_require__(638);
 
+      /***/
+    },
+
+    /***/ 8217: /***/ function(
+      __unused_webpack_module,
+      exports,
+      __webpack_require__
+    ) {
+      "use strict";
+      /** @license React vundefined
+       * use-subscription.production.min.js
+       *
+       * Copyright (c) Facebook, Inc. and its affiliates.
+       *
+       * This source code is licensed under the MIT license found in the
+       * LICENSE file in the root directory of this source tree.
+       */
+      var e = __webpack_require__(6086),
+        g = __webpack_require__(7294);
+      exports.useSubscription = function(a) {
+        var c = a.getCurrentValue,
+          d = a.subscribe,
+          b = g.useState(function() {
+            return { getCurrentValue: c, subscribe: d, value: c() };
+          });
+        a = b[0];
+        var f = b[1];
+        b = a.value;
+        if (a.getCurrentValue !== c || a.subscribe !== d)
+          (b = c()), f({ getCurrentValue: c, subscribe: d, value: b });
+        g.useDebugValue(b);
+        g.useEffect(
+          function() {
+            function b() {
+              if (!a) {
+                var b = c();
+                f(function(a) {
+                  return a.getCurrentValue !== c ||
+                    a.subscribe !== d ||
+                    a.value === b
+                    ? a
+                    : e({}, a, { value: b });
+                });
+              }
+            }
+            var a = !1,
+              h = d(b);
+            b();
+            return function() {
+              a = !0;
+              h();
+            };
+          },
+          [c, d]
+        );
+        return b;
+      };
+
+      /***/
+    },
+
+    /***/ 7161: /***/ function(
+      module,
+      __unused_webpack_exports,
+      __webpack_require__
+    ) {
+      "use strict";
+
+      if (true) {
+        module.exports = __webpack_require__(8217);
+      } else {
+      }
+
       /***/
     }
   },
Diff for framework-HASH.js
@@ -8688,76 +8688,6 @@
       } else {
       }
 
-      /***/
-    },
-
-    /***/ 8217: /***/ function(
-      __unused_webpack_module,
-      exports,
-      __webpack_require__
-    ) {
-      /** @license React vundefined
-       * use-subscription.production.min.js
-       *
-       * Copyright (c) Facebook, Inc. and its affiliates.
-       *
-       * This source code is licensed under the MIT license found in the
-       * LICENSE file in the root directory of this source tree.
-       */
-      var e = __webpack_require__(6086),
-        g = __webpack_require__(7294);
-      exports.useSubscription = function(a) {
-        var c = a.getCurrentValue,
-          d = a.subscribe,
-          b = g.useState(function() {
-            return { getCurrentValue: c, subscribe: d, value: c() };
-          });
-        a = b[0];
-        var f = b[1];
-        b = a.value;
-        if (a.getCurrentValue !== c || a.subscribe !== d)
-          (b = c()), f({ getCurrentValue: c, subscribe: d, value: b });
-        g.useDebugValue(b);
-        g.useEffect(
-          function() {
-            function b() {
-              if (!a) {
-                var b = c();
-                f(function(a) {
-                  return a.getCurrentValue !== c ||
-                    a.subscribe !== d ||
-                    a.value === b
-                    ? a
-                    : e({}, a, { value: b });
-                });
-              }
-            }
-            var a = !1,
-              h = d(b);
-            b();
-            return function() {
-              a = !0;
-              h();
-            };
-          },
-          [c, d]
-        );
-        return b;
-      };
-
-      /***/
-    },
-
-    /***/ 7161: /***/ function(
-      module,
-      __unused_webpack_exports,
-      __webpack_require__
-    ) {
-      if (true) {
-        module.exports = __webpack_require__(8217);
-      } else {
-      }
-
       /***/
     }
   }
Diff for index.html
@@ -15,7 +15,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/framework-535325b9235a831f.js"
+      src="/_next/static/chunks/framework-287ad9396a03b869.js"
       defer=""
     ></script>
     <script
Diff for link.html
@@ -15,7 +15,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/framework-535325b9235a831f.js"
+      src="/_next/static/chunks/framework-287ad9396a03b869.js"
       defer=""
     ></script>
     <script
Diff for withRouter.html
@@ -15,7 +15,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/framework-535325b9235a831f.js"
+      src="/_next/static/chunks/framework-287ad9396a03b869.js"
       defer=""
     ></script>
     <script
Commit: 700135d

Copy link
Member

@ijjk ijjk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

@ijjk ijjk dismissed sokra’s stale review February 6, 2022 22:38

feedback addressed

@kodiakhq kodiakhq bot merged commit 6d0bd20 into vercel:canary Feb 6, 2022
natew pushed a commit to natew/next.js that referenced this pull request Feb 16, 2022
…pack-config (vercel#31264)

Fixes: vercel#31240
Closes: vercel#32324
Adding a try-catch block to handle situations when packages are found at relative path in getPackagePath function. This is likely to occur when using `preact` instead of `react-dom`, as `scheduler` package will not be found wrt `react-dom`

## Bug

- [x] Related issues linked using `fixes vercel#31240`

Co-authored-by: JJ Kasper <22380829+ijjk@users.noreply.github.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[12.0.4-canary.2], Preact - Local dev server fails with Error: Cannot find module 'scheduler/package.json'
6 participants