Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

.pnpmfile change not triger resolution #6726

Closed
tjx666 opened this issue Jun 27, 2023 · 9 comments
Closed

.pnpmfile change not triger resolution #6726

tjx666 opened this issue Jun 27, 2023 · 9 comments

Comments

@tjx666
Copy link

tjx666 commented Jun 27, 2023

pnpm version: 8.6.3

Code to reproduce the issue:

https://stackblitz.com/edit/node-3neeb1?file=.pnpmfile.cjs

  1. pnpm install
  2. change log statement in .pnpmfile.cjs
  3. pnpm install, you will see Lockfile is up to date, resolution step is skipped

Expected behavior:

change .pnpmfile.cjs should always trigger resolution

Actual behavior:

as title

Additional information:

@zkochan
Copy link
Member

zkochan commented Jul 2, 2023

How can we know if the file has changed? We could store the hash of the file in the lockfile. However, the hash won't change if the modification will happen not in .pnpmfile.cjs itself, but in modules that it requires.

@tjx666
Copy link
Author

tjx666 commented Jul 2, 2023

at least change .pnpmfile.cjs should trigger resolution

@thynson thynson changed the title .pnpmfile change not triiger resolution .pnpmfile change not triger resolution Jul 3, 2023
@thynson
Copy link
Sponsor Contributor

thynson commented Jul 3, 2023

Let me try to understand the point of @tjx666 . The pnpmfile file affects resolution, since it can mutate the dependencies of a package in hooks, so it's reasonable that any modification to it need to trigger an resolution. The best way of it can be store the hash of pnpmfile in the lockfile, and possibly the file path of it, if renaming a pnpmfile, e.g., from pnpmfile.js to pnpmfile.js hit a corner case.

@icy0307
Copy link

icy0307 commented Jul 5, 2023

#4794
this is the same issue.
Every time someone modifies package.json first, the hooks on master branch become useless, until the next time someone installs or removes something in package.json
Can we do some ast parsing recursively and find all related files? Store the hash for pnpmfile and related "required" files.

@thynson
Copy link
Sponsor Contributor

thynson commented Jul 5, 2023

Let me try to understand the point of @tjx666 . The pnpmfile file affects resolution, since it can mutate the dependencies of a package in hooks, so it's reasonable that any modification to it need to trigger an resolution. The best way of it can be store the hash of pnpmfile in the lockfile, and possibly the file path of it, if renaming a pnpmfile, e.g., from pnpmfile.js to pnpmfile.js hit a corner case.

I realized storing the hash value of pnpmfile.js in the lockfile is a bad idea. Instead, make sure the hooks in the pnpmfile.js are invoked for each dependencies in package.json, before checking a lockfile is up-to-date.

@icy0307
Copy link

icy0307 commented Jul 6, 2023

But doesn't the readPackage hook get called during the resolution step?
@thynson

@thynson
Copy link
Sponsor Contributor

thynson commented Jul 6, 2023

@icy0307 It seems like not for every dependency, according to #4794 (comment).

@icy0307
Copy link

icy0307 commented Jul 6, 2023

@thynson yes, not invoke for every dependency only if resolution get skipped.
And this because current hooks get called in resolution step. But we need to call every hooks to know if the package lock is up-to-date.
That said, currently, we need hooks which called in resolution step to know if we can skip resolution, which is impossible.
We need to find another way to know if resolution can be skipped if pnpmfile changes. @thynson

@zkochan
Copy link
Member

zkochan commented Feb 23, 2024

Fixed by #7662

@zkochan zkochan closed this as completed Feb 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants