search

nodejs / corepack

Zero-runtime-dependency package acting as bridge between Node projects and their package managers
MIT License
2.31k stars 146 forks source link

Separate read and write operations on lastKnownGood.json #446

Closed das7pad closed 2 months ago

das7pad commented 3 months ago

Description

Closes https://github.com/nodejs/corepack/issues/183 Closes https://github.com/nodejs/corepack/issues/422

This PR is separating read and write operations on lastKnownGood.json. It will also skip overwriting lastKnownGood.json with the same content.

Review

This PR is best reviewed with white space changes ignored due to indentation changes from removing try/catch/finally blocks.

I refactored the read and write operations into self contained functions. The re-use of a r+ file handle does not work anymore when separating the two kinds of operations.

The content of lastKnownGood.json is validated/sanitized when reading. Any consumers can operate on the Record<string, string> object directly. This greatly simplified the logic in activatePackageManager and getLastKnownGoodFromFileContent.

I could not get the "per-project" install to work fully offline/read-only in a straightforward way (just running corepack install). Running corepack install only caches the requested binary from package.json -> packageManager. When invoking the package-manager (yarn) via corepack yarn, corepack tries to discover the latest version to prepare a fallbackLocator in Engine.executePackageManagerRequest first, but without a lastKnownGood.json (corepack install does not prepare it), it attempts a network request. The "code fix" here is to defer the fallbackLocator work until after discovering any package.json file. This is a major undertaking and something for another day. I left a note for this in a comment in the "per-project" test, next to the workaround: running corepack yarn --version once populates the lastKnownGood.json cache.

aduh95 commented 2 months ago

corepack tries to discover the latest version to prepare a fallbackLocator in Engine.executePackageManagerRequest first, but without a lastKnownGood.json (corepack install does not prepare it), it attempts a network request

I wonder if https://github.com/nodejs/corepack/pull/432/files#diff-ac2e60980d41d74acab8d4ec9d41b6bd52b08f056b3ea65f89952de64eca3601 did not fix that already.