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

.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_INSTALL_SCRIPTS = [`preinstall`, `install`, `postinstall`];
 
 export class NpmSemverResolver implements Resolver {
   supportsDescriptor(descriptor: Descriptor, opts: MinimalResolveOptions) {
@@ -152,10 +153,15 @@ 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.
-    if (!manifest.dependencies.has(NODE_GYP_IDENT.identHash) && !manifest.peerDependencies.has(NODE_GYP_IDENT.identHash)) {
+    // Also, node-gyp is not always set as a dependency in packages, so it will also be added
+    // if used in package scripts that may be reached from an install lifecycle script.
+    if (
+      !manifest.dependencies.has(NODE_GYP_IDENT.identHash) &&
+      !manifest.peerDependencies.has(NODE_GYP_IDENT.identHash) &&
+      NODE_GYP_INSTALL_SCRIPTS.some(scriptName => manifest.scripts.has(scriptName))
+    ) {
       for (const value of manifest.scripts.values()) {
-        if (value.match(NODE_GYP_MATCH)) {
+        if (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,71 @@ 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);
+    });
+
+    it(`should inject node-gyp when an install script delegates to another script using it`, async () => {
+      mockPackageMetadata({
+        build: `node-gyp rebuild`,
+        install: `yarn build`,
+      });
+
+      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);
+    });
+  });
 });

yarn.lock

@@ -15929,8 +15929,6 @@ __metadata:
 "node-addon-api@npm:^4.3.0":
   version: 4.3.0
   resolution: "node-addon-api@npm:4.3.0"
-  dependencies:
-    node-gyp: "npm:latest"
   checksum: 10/d3b38d16cb9ad0714d965331d0e38cef1c27750c2c3343cd3464a9ed8158501a2910ccbf2fd9fdc476e806a19dbc9e0524ff9d66a7c779d42a9752a63ba30b80
   languageName: node
   linkType: hard