fix: don't use FileFetcher.manifest() for git pkgs

[chmeliik]

To get the manifest for a hosted, resolved git package, pacote uses FileFetcher.manifest. FileFetcher.manifest calls extract(), which calls _tarballFromResolved.

This means getting the manifest involves nearly a complete pack operation, including 'npm install', the 'prepare' script etc. It also means getting the manifest requires an Arborist constructor (causes npm/cli#6723).

Remove the special handling of hosted, resolved git packages, handle all git packages the same way: clone the repo (or get the tarball from a hosted git server) and read the package.json.

The effects for hosted, resolved git packages:

• getting the manifest no longer involves packing the tarball
• getting the manifest no longer requires an Arborist constructor
• the manifest no longer includes an _integrity (same as un-hosted or un-resolved git packages)

[chmeliik]

Hi! I tried to fix an issue which causes npm pack to fail for resolved, hosted git packages

There are some potential BREAKING CHANGE candidates here, not sure if they really are breaking changes:

• When getting the manifest, pacote no longer executes the prepare script. Technically, the script can modify package.json => by not executing it, the result of manifest() can be different
  • But this doesn't seem like something that was ever taken into account, un-hosted or un-resolved git packages don't execute the script either
• The manifest no longer includes an _integrity (same as un-hosted or un-resolved git packages)
  • As far as I understand, pacote is not supposed to include integrity for any git packages

[wraithgar]

The reason it does this is a historical assumption that the git source of a package needs to be prepared, i.e. it's the "source".

_integrity is not supposed to be used for git sources, so its presence here is immaterial, no worries there.

The real sticking point is going to be if folks use prepare to alter the package.json. This is quite common. A change like this would need to be part of a breaking change.

ETA: it looks like the integrity you were talking about was the integrity from the untar operation of the git repo itself, it is not the same as the integrity that is used in the package lock.

[chmeliik]

The real sticking point is going to be if folks use prepare to alter the package.json. This is quite common. A change like this would need to be part of a breaking change.

It might be worth highlighting that prepare only runs for "resolved" specs. To take the example from the npm issue:

npm pack vercel/ms#3.0.0-canary.1
npm pack vercel/ms#1304f150b38027e0818cc122106b5c7322d68d0c 

^ these are the same package, but only the one with the commit hash would get to run the prepare script if it had one

Though I understand this may not be enough of an argument against this being a breaking change.

It is also possible to fix the npm pack issue on the npm side, simply by passing an Arborist constructor to all manifest() and packument() calls that might be dealing with git packages. Would that be prefered?

[chmeliik]

to clarify: only the one with the commit hash would get to run the prepare script while getting the manifest / the packument

all git deps get to run prepare while packing the tarball

[wraithgar]

It is also possible to fix the npm pack issue on the npm side, simply by passing an Arborist constructor to all manifest() and packument() calls that might be dealing with git packages. Would that be prefered?

Yes at a bare minimum we want to do this, even if we eventually make a breaking change going forward.

[chmeliik]

It is also possible to fix the npm pack issue on the npm side, simply by passing an Arborist constructor to all manifest() and packument() calls that might be dealing with git packages. Would that be prefered?

Yes at a bare minimum we want to do this, even if we eventually make a breaking change going forward.

Got it, I'll look into that.