feat(pnp): better builtin not found error with considerBuiltins disab…

…led (#1695) * feat(pnp): better builtin not found error with considerBuiltins disabled * chore: unbump std version * chore: update pnp hook * Tweaks Co-authored-by: Maël Nison <[email protected]>
yarnpkg · Aug 13, 2020 · 3c71ffa · 3c71ffa
1 parent 95af161
commit 3c71ffa
Show file tree

Hide file tree

Showing 7 changed files with 145 additions and 46 deletions.
diff --git a/.pnp.js b/.pnp.js
diff --git a/.yarn/versions/078238f6.yml b/.yarn/versions/078238f6.yml
@@ -0,0 +1,24 @@
+releases:
+  "@yarnpkg/cli": prerelease
+  "@yarnpkg/plugin-pnp": minor
+  "@yarnpkg/pnp": minor
+
+declined:
+  - "@yarnpkg/plugin-compat"
+  - "@yarnpkg/plugin-constraints"
+  - "@yarnpkg/plugin-dlx"
+  - "@yarnpkg/plugin-essentials"
+  - "@yarnpkg/plugin-init"
+  - "@yarnpkg/plugin-interactive-tools"
+  - "@yarnpkg/plugin-node-modules"
+  - "@yarnpkg/plugin-npm-cli"
+  - "@yarnpkg/plugin-pack"
+  - "@yarnpkg/plugin-patch"
+  - "@yarnpkg/plugin-stage"
+  - "@yarnpkg/plugin-typescript"
+  - "@yarnpkg/plugin-version"
+  - "@yarnpkg/plugin-workspace-tools"
+  - "@yarnpkg/builder"
+  - "@yarnpkg/core"
+  - "@yarnpkg/doctor"
+  - "@yarnpkg/pnpify"
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -32,6 +32,10 @@
 
 - Scripts can now use the `$RANDOM` variable as well as simple calculations using `+`, `-`, `*`, `/` and `()` inside `$(())`
 
+### Third-party integrations
+
+- The PnP hook will now display clearer error message when requiring Node builtins from contexts that can't access them out of the box (for example when accessing the `fs` module from within a Webpack browser bundle).
+
 ## 2.1.1
 
 - Fixed hyperlink rendering on iTerm

diff --git a/packages/acceptance-tests/pkg-tests-specs/sources/pnpapi.test.js b/packages/acceptance-tests/pkg-tests-specs/sources/pnpapi.test.js
@@ -434,4 +434,24 @@ describe(`Plug'n'Play API`, () => {
       );
     });
   });
+
+  describe(`Semantic Errors`, () => {
+    test(
+      `it should throw detailed errors when a builtin is not found and the 'considerBuiltins' option is set to false`,
+      makeTemporaryEnv(
+        {},
+        async ({path, run, source}) => {
+          await run(`install`);
+
+          await expect(source(`require('pnpapi').resolveRequest('fs', ${JSON.stringify(`${npath.fromPortablePath(path)}/`)}, {considerBuiltins: false})`)).rejects.toMatchObject({
+            externalException: {
+              message: expect.stringContaining(`Your application tried to access fs. While this module is usually interpreted as a Node builtin,`),
+              code: `MODULE_NOT_FOUND`,
+              pnpCode: `UNDECLARED_DEPENDENCY`,
+            },
+          });
+        },
+      ),
+    );
+  });
 });
diff --git a/packages/gatsby/content/advanced/pnp-api.md b/packages/gatsby/content/advanced/pnp-api.md
@@ -121,7 +121,7 @@ Note that the `pnpapi` builtin is *contextual*: while two packages from the same
 export const VERSIONS: {std: number, [key: string]: number};
 ```
 
-The `VERSIONS` object contains a set of numbers that detail which version of the API is currently exposed. The only version that is guaranteed to be there is `std`, which will refer to the version of this document. Other keys are meant to be used to describe extensions provided by third-party implementors.
+The `VERSIONS` object contains a set of numbers that detail which version of the API is currently exposed. The only version that is guaranteed to be there is `std`, which will refer to the version of this document. Other keys are meant to be used to describe extensions provided by third-party implementors. Versions will only be bumped when the signatures of the public API change.
 
 **Note:** The current version is 3. We bump it responsibly and strive to make each version backward-compatible with the previous ones, but as you can probably guess some features are only available with the latest versions.
 

diff --git a/packages/yarnpkg-pnp/sources/hook.js b/packages/yarnpkg-pnp/sources/hook.js
diff --git a/packages/yarnpkg-pnp/sources/loader/makeApi.ts b/packages/yarnpkg-pnp/sources/loader/makeApi.ts
@@ -667,18 +667,34 @@ export function makeApi(runtimeState: RuntimeState, opts: MakeApiOptions): PnpAp
           }
         }
       } else if (dependencyReference === undefined) {
-        if (isDependencyTreeRoot(issuerLocator)) {
-          error = makeError(
-            ErrorCode.UNDECLARED_DEPENDENCY,
-            `Your application tried to access ${dependencyName}, but it isn't declared in your dependencies; this makes the require call ambiguous and unsound.\n\nRequired package: ${dependencyName} (via "${requestForDisplay}")\nRequired by: ${issuerForDisplay}\n`,
-            {request: requestForDisplay, issuer: issuerForDisplay, dependencyName},
-          );
+        if (!considerBuiltins && builtinModules.has(request)) {
+          if (isDependencyTreeRoot(issuerLocator)) {
+            error = makeError(
+              ErrorCode.UNDECLARED_DEPENDENCY,
+              `Your application tried to access ${dependencyName}. While this module is usually interpreted as a Node builtin, your resolver is running inside a non-Node resolution context where such builtins are ignored. Since ${dependencyName} isn't otherwise declared in your dependencies, this makes the require call ambiguous and unsound.\n\nRequired package: ${dependencyName} (via "${requestForDisplay}")\nRequired by: ${issuerForDisplay}\n`,
+              {request: requestForDisplay, issuer: issuerForDisplay, dependencyName},
+            );
+          } else {
+            error = makeError(
+              ErrorCode.UNDECLARED_DEPENDENCY,
+              `${issuerLocator.name} tried to access ${dependencyName}. While this module is usually interpreted as a Node builtin, your resolver is running inside a non-Node resolution context where such builtins are ignored. Since ${dependencyName} isn't otherwise declared in ${issuerLocator.name}'s dependencies, this makes the require call ambiguous and unsound.\n\nRequired package: ${dependencyName} (via "${requestForDisplay}")\nRequired by: ${issuerForDisplay}\n`,
+              {request: requestForDisplay, issuer: issuerForDisplay, issuerLocator: Object.assign({}, issuerLocator), dependencyName},
+            );
+          }
         } else {
-          error = makeError(
-            ErrorCode.UNDECLARED_DEPENDENCY,
-            `${issuerLocator.name} tried to access ${dependencyName}, but it isn't declared in its dependencies; this makes the require call ambiguous and unsound.\n\nRequired package: ${dependencyName} (via "${requestForDisplay}")\nRequired by: ${issuerLocator.name}@${issuerLocator.reference} (via ${issuerForDisplay})\n`,
-            {request: requestForDisplay, issuer: issuerForDisplay, issuerLocator: Object.assign({}, issuerLocator), dependencyName},
-          );
+          if (isDependencyTreeRoot(issuerLocator)) {
+            error = makeError(
+              ErrorCode.UNDECLARED_DEPENDENCY,
+              `Your application tried to access ${dependencyName}, but it isn't declared in your dependencies; this makes the require call ambiguous and unsound.\n\nRequired package: ${dependencyName} (via "${requestForDisplay}")\nRequired by: ${issuerForDisplay}\n`,
+              {request: requestForDisplay, issuer: issuerForDisplay, dependencyName},
+            );
+          } else {
+            error = makeError(
+              ErrorCode.UNDECLARED_DEPENDENCY,
+              `${issuerLocator.name} tried to access ${dependencyName}, but it isn't declared in its dependencies; this makes the require call ambiguous and unsound.\n\nRequired package: ${dependencyName} (via "${requestForDisplay}")\nRequired by: ${issuerLocator.name}@${issuerLocator.reference} (via ${issuerForDisplay})\n`,
+              {request: requestForDisplay, issuer: issuerForDisplay, issuerLocator: Object.assign({}, issuerLocator), dependencyName},
+            );
+          }
         }
       }