fix(plugin-npm): limit implicit node-gyp checks to build scripts

Author: ychampion

.yarn/versions/plugin-npm-node-gyp-build-scripts.yml

@@ -0,0 +1,8 @@
+releases:
+  "@yarnpkg/plugin-npm": patch
+
+declined:
+  - "@yarnpkg/cli"
+  - "@yarnpkg/core"
+  - "@yarnpkg/plugin-compat"
+  - "@yarnpkg/plugin-npm-cli"

packages/plugin-npm/sources/NpmSemverResolver.ts

@@ -11,6 +11,7 @@ import * as npmHttpUtils
 
 const NODE_GYP_IDENT = structUtils.makeIdent(null, `node-gyp`);
 const NODE_GYP_MATCH = /\b(node-gyp|prebuild-install)\b/;
+const NODE_GYP_BUILD_SCRIPTS = [`preinstall`, `install`, `postinstall`];
 
 export class NpmSemverResolver implements Resolver {
   supportsDescriptor(descriptor: Descriptor, opts: MinimalResolveOptions) {
@@ -152,10 +153,12 @@ export class NpmSemverResolver implements Resolver {
     // Manually add node-gyp dependency if there is a script using it and not already set
     // This is because the npm registry will automatically add a `node-gyp rebuild` install script
     // in the metadata if there is not already an install script and a binding.gyp file exists.
-    // Also, node-gyp is not always set as a dependency in packages, so it will also be added if used in scripts.
+    // Also, node-gyp is not always set as a dependency in packages, so it will also be added if used in build scripts.
     if (!manifest.dependencies.has(NODE_GYP_IDENT.identHash) && !manifest.peerDependencies.has(NODE_GYP_IDENT.identHash)) {
-      for (const value of manifest.scripts.values()) {
-        if (value.match(NODE_GYP_MATCH)) {
+      for (const scriptName of NODE_GYP_BUILD_SCRIPTS) {
+        const value = manifest.scripts.get(scriptName);
+
+        if (typeof value !== `undefined` && NODE_GYP_MATCH.test(value)) {
           manifest.dependencies.set(NODE_GYP_IDENT.identHash, structUtils.makeDescriptor(NODE_GYP_IDENT, `latest`));
           break;
         }

packages/plugin-npm/tests/NpmSemverResolver.test.ts

@@ -1,6 +1,15 @@
-import {structUtils}       from '@yarnpkg/core';
+jest.mock(`../sources/npmHttpUtils`, () => ({
+  getPackageMetadata: jest.fn(),
+}));
 
-import {NpmSemverResolver} from '../sources/NpmSemverResolver';
+import {structUtils} from '@yarnpkg/core';
+
+const {NpmSemverResolver}: typeof import('../sources/NpmSemverResolver') = require(`../sources/NpmSemverResolver`);
+const npmHttpUtils: typeof import('../sources/npmHttpUtils') = require(`../sources/npmHttpUtils`);
+
+afterEach(() => {
+  jest.clearAllMocks();
+});
 
 describe(`NpmSemverResolver`, () => {
   describe(`getSatisfying`, () => {
@@ -22,4 +31,57 @@ describe(`NpmSemverResolver`, () => {
       expect(results.locators[0].locatorHash).toEqual(locator.locatorHash);
     });
   });
+
+  describe(`resolve`, () => {
+    const ident = structUtils.makeIdent(null, `native-package`);
+    const nodeGypIdent = structUtils.makeIdent(null, `node-gyp`);
+
+    const makeResolveOptions = () => ({
+      project: {
+        configuration: {
+          normalizeDependencyMap: (dependencies: Map<any, any>) => dependencies,
+        },
+      },
+    } as any);
+
+    const mockPackageMetadata = (scripts: Record<string, string>) => {
+      const getPackageMetadata = npmHttpUtils.getPackageMetadata as jest.MockedFunction<typeof npmHttpUtils.getPackageMetadata>;
+
+      getPackageMetadata.mockResolvedValue({
+        versions: {
+          [`1.0.0`]: {
+            name: structUtils.stringifyIdent(ident),
+            version: `1.0.0`,
+            scripts,
+          },
+        },
+      } as any);
+    };
+
+    it(`shouldn't inject node-gyp when only a non-build script uses it`, async () => {
+      mockPackageMetadata({
+        test: `node-gyp rebuild`,
+      });
+
+      const resolver = new NpmSemverResolver();
+      const locator = structUtils.makeLocator(ident, `npm:1.0.0`);
+
+      const pkg = await resolver.resolve(locator, makeResolveOptions());
+
+      expect(pkg.dependencies.has(nodeGypIdent.identHash)).toEqual(false);
+    });
+
+    it(`should inject node-gyp when an install script uses it`, async () => {
+      mockPackageMetadata({
+        install: `node-gyp rebuild`,
+      });
+
+      const resolver = new NpmSemverResolver();
+      const locator = structUtils.makeLocator(ident, `npm:1.0.0`);
+
+      const pkg = await resolver.resolve(locator, makeResolveOptions());
+
+      expect(pkg.dependencies.has(nodeGypIdent.identHash)).toEqual(true);
+    });
+  });
 });