Hooks provided in opts are lost in processing by install command

[AGrzes]

I was trying to get pnpm deploy (beta) feature to work and correctly "inject" my workspace modules and I noticed that they are not injected due to readPackage hook not being applied.

Here

pnpm/packages/pnpmfile/src/requireHooks.ts

Line 55 in b6f788c

if (!globalHooks && !hooks) return {}

empty object is returned if neither globalHooks nor hooks are set.
Simple change to

if (!globalHooks && !hooks) return opts.hooks || {}

Would prevent hooks set in opts from being lost.

f the change would be acceptable I will provide PR with tests.

pnpm version:

7.9.5

Code to reproduce the issue:

I can provide it if the description is not enough.

Expected behavior:

When hooks are provided in opts they are returned from requireHooks function

Actual behavior:

Hooks provided in opts are lost.

Additional information:

• node -v prints: v14.20.0
• Windows, macOS, or Linux?: Linux

[AGrzes]

Actually after working on PR I found out that the hooks provided in opts are not only ignored if there ar no local/ global hooks - they are always ignored. My PR addresses this issue and I would be grateful for some feedback - as I confirmed that changed version resolves my issue with workspace package not being "injected" when using deploy command.

[zkochan]

I think we should do some refactoring. Probably @pnpm/pnpmfile should not merge the hooks and just return an array of hooks. Then the deploy command would just append a new hooks object to the array. Then later before usage (maybe in @pnpm/core) the hooks can be piped together. This part is already partially implemented:

pnpm/packages/core/src/install/index.ts

Lines 614 to 621 in f3fc6b5

	const readPackageAndExtend = hooks.length === 1
	? hooks[0]
	: pipeWith(async (f, res) => f(await res), hooks as any) as ReadPackageHook // eslint-disable-line @typescript-eslint/no-explicit-any
	if (readPackageHook != null) {
	return (async (manifest: ProjectManifest, dir?: string) => readPackageAndExtend(await readPackageHook(manifest, dir), dir)) as ReadPackageHook
	}
	return readPackageAndExtend
	}

[AGrzes]

I will look into this - if I understand You correctly you suggest that

• @pnpm/pnpmfile would return array of hooks for hooks that can be "merged" later
• the array is passed correctly to createReadPackageHook
• hook from options is also passed in and handled by createReadPackageHook
  Am I right?

[zkochan]

Sounds correct.

[AGrzes]

What I found out the issue happens only with option shared-workspace-lockfile = false - with shared workspace lock the hook is handled correctly.

[AGrzes]

The line

pnpm/packages/plugin-commands-installation/src/recursive.ts

Line 292 in f3fc6b5

const hooks = opts.ignorePnpmfile ? {} : requireHooks(rootDir, opts)

coupled with

pnpm/packages/plugin-commands-installation/src/recursive.ts

Line 354 in f3fc6b5

hooks,

Causes hooks from opts to be discarded in favor of pnpmfile generated hooks before they can reach createReadPackageHook.

[AGrzes]

@zkochan one question - I found out that config module also uses requireHooks

pnpm/packages/config/src/index.ts

Line 525 in f3fc6b5

pnpmConfig.hooks = requireHooks(pnpmConfig.lockfileDir ?? pnpmConfig.dir, pnpmConfig)

And it is very hard for me to trace places where change in type (to array) could have ripple effects - so I'm worried about changing type of readPackage hook to array in between requireHooks and createReadPackageHook because I'm not sure the flow will always lead to the later.

[zkochan]

You will get typing errors, just run pnpm -w compile and you'll be able to find all places that need to be changed.