Skip to content

Commit ae53f78

Browse files
committed
Preserve delegated native build detection
Constraint: Maintainer pointed out install scripts can delegate to other package scripts. Rejected: Only scan preinstall/install/postinstall bodies | Misses install scripts that call build scripts using node-gyp. Confidence: high Scope-risk: narrow Directive: Keep test-only node-gyp references ignored unless an install lifecycle script makes package scripts reachable during install. Tested: corepack yarn jest packages/plugin-npm/tests/NpmSemverResolver.test.ts --runInBand; corepack yarn jest packages/plugin-npm/tests --runInBand; corepack yarn test:lint; corepack yarn constraints; corepack yarn version check; corepack yarn dedupe --check; corepack yarn typecheck:all; YARN_ENABLE_HARDENED_MODE=1 corepack yarn install --immutable --immutable-cache; git diff --check. Not-tested: Netlify docs build, already failing on upstream master for an unrelated schema example issue.
1 parent 45bfe9c commit ae53f78

2 files changed

Lines changed: 24 additions & 7 deletions

File tree

packages/plugin-npm/sources/NpmSemverResolver.ts

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ import * as npmHttpUtils
1111

1212
const NODE_GYP_IDENT = structUtils.makeIdent(null, `node-gyp`);
1313
const NODE_GYP_MATCH = /\b(node-gyp|prebuild-install)\b/;
14-
const NODE_GYP_BUILD_SCRIPTS = [`preinstall`, `install`, `postinstall`];
14+
const NODE_GYP_INSTALL_SCRIPTS = [`preinstall`, `install`, `postinstall`];
1515

1616
export class NpmSemverResolver implements Resolver {
1717
supportsDescriptor(descriptor: Descriptor, opts: MinimalResolveOptions) {
@@ -153,12 +153,15 @@ export class NpmSemverResolver implements Resolver {
153153
// Manually add node-gyp dependency if there is a script using it and not already set
154154
// This is because the npm registry will automatically add a `node-gyp rebuild` install script
155155
// in the metadata if there is not already an install script and a binding.gyp file exists.
156-
// Also, node-gyp is not always set as a dependency in packages, so it will also be added if used in build scripts.
157-
if (!manifest.dependencies.has(NODE_GYP_IDENT.identHash) && !manifest.peerDependencies.has(NODE_GYP_IDENT.identHash)) {
158-
for (const scriptName of NODE_GYP_BUILD_SCRIPTS) {
159-
const value = manifest.scripts.get(scriptName);
160-
161-
if (typeof value !== `undefined` && NODE_GYP_MATCH.test(value)) {
156+
// Also, node-gyp is not always set as a dependency in packages, so it will also be added
157+
// if used in package scripts that may be reached from an install lifecycle script.
158+
if (
159+
!manifest.dependencies.has(NODE_GYP_IDENT.identHash) &&
160+
!manifest.peerDependencies.has(NODE_GYP_IDENT.identHash) &&
161+
NODE_GYP_INSTALL_SCRIPTS.some(scriptName => manifest.scripts.has(scriptName))
162+
) {
163+
for (const value of manifest.scripts.values()) {
164+
if (NODE_GYP_MATCH.test(value)) {
162165
manifest.dependencies.set(NODE_GYP_IDENT.identHash, structUtils.makeDescriptor(NODE_GYP_IDENT, `latest`));
163166
break;
164167
}

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

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,5 +83,19 @@ describe(`NpmSemverResolver`, () => {
8383

8484
expect(pkg.dependencies.has(nodeGypIdent.identHash)).toEqual(true);
8585
});
86+
87+
it(`should inject node-gyp when an install script delegates to another script using it`, async () => {
88+
mockPackageMetadata({
89+
build: `node-gyp rebuild`,
90+
install: `yarn build`,
91+
});
92+
93+
const resolver = new NpmSemverResolver();
94+
const locator = structUtils.makeLocator(ident, `npm:1.0.0`);
95+
96+
const pkg = await resolver.resolve(locator, makeResolveOptions());
97+
98+
expect(pkg.dependencies.has(nodeGypIdent.identHash)).toEqual(true);
99+
});
86100
});
87101
});

0 commit comments

Comments
 (0)