From 3db0db8d12aed3bd2a396b7ac4a4be066b340113 Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Wed, 22 Nov 2023 02:01:27 +0200 Subject: [PATCH] revert: "feat: use-experimental-npmjs-files-index option (#7177)" This reverts commit a2bf6a0e96b2a902ada29995e1427a56a74a82b6. --- .changeset/lovely-islands-hide.md | 10 --- config/config/src/Config.ts | 1 - config/config/src/index.ts | 2 - cspell.json | 2 +- .../src/index.ts | 3 +- .../core/src/getPeerDependencyIssues.ts | 2 - .../core/src/install/extendInstallOptions.ts | 2 - pkg-manager/core/src/install/index.ts | 1 - pkg-manager/core/test/install/lockfileOnly.ts | 10 --- .../src/install.ts | 1 - .../src/installDeps.ts | 1 - .../src/recursive.ts | 1 - pkg-manager/resolve-dependencies/package.json | 1 - .../src/checkViaRegistryIfPkgRequiresBuild.ts | 68 ------------------- pkg-manager/resolve-dependencies/src/index.ts | 18 ++--- .../src/resolveDependencies.ts | 7 +- .../src/resolveDependencyTree.ts | 2 - .../resolve-dependencies/tsconfig.json | 3 - pnpm-lock.yaml | 3 - 19 files changed, 16 insertions(+), 122 deletions(-) delete mode 100644 .changeset/lovely-islands-hide.md delete mode 100644 pkg-manager/resolve-dependencies/src/checkViaRegistryIfPkgRequiresBuild.ts diff --git a/.changeset/lovely-islands-hide.md b/.changeset/lovely-islands-hide.md deleted file mode 100644 index 6f4f7e0f75..0000000000 --- a/.changeset/lovely-islands-hide.md +++ /dev/null @@ -1,10 +0,0 @@ ---- -"@pnpm/config": minor -"@pnpm/core": minor -"@pnpm/exec.files-include-install-scripts": patch -"@pnpm/plugin-commands-installation": minor -"@pnpm/resolve-dependencies": minor -"pnpm": minor ---- - -(EXPERIMENTAL) When the `use-experimental-npmjs-files-index` option is set to `true` and `--lockfile-only` installation is performed, package tarballs are not downloaded. npm's beta file index feature is used to populate the lockfile [#7117](https://github.com/pnpm/pnpm/pull/7177). diff --git a/config/config/src/Config.ts b/config/config/src/Config.ts index eeb9012803..4bc07157d0 100644 --- a/config/config/src/Config.ts +++ b/config/config/src/Config.ts @@ -162,7 +162,6 @@ export interface Config { ignoreWorkspaceCycles?: boolean disallowWorkspaceCycles?: boolean packGzipLevel?: number - useExperimentalNpmjsFilesIndex?: boolean registries: Registries ignoreWorkspaceRootCheck: boolean diff --git a/config/config/src/index.ts b/config/config/src/index.ts index 10f3424ebb..eb87cd3ec0 100644 --- a/config/config/src/index.ts +++ b/config/config/src/index.ts @@ -114,7 +114,6 @@ export const types = Object.assign({ 'resolve-peers-from-workspace-root': Boolean, 'aggregate-output': Boolean, 'reporter-hide-prefix': Boolean, - 'use-experimental-npmjs-files-index': Boolean, 'save-peer': Boolean, 'save-workspace-protocol': Boolean, 'script-shell': String, @@ -243,7 +242,6 @@ export async function getConfig ( registry: npmDefaults.registry, 'resolution-mode': 'highest', 'resolve-peers-from-workspace-root': true, - 'use-experimental-npmjs-files-index': false, 'save-peer': false, 'save-workspace-protocol': 'rolling', 'scripts-prepend-node-path': false, diff --git a/cspell.json b/cspell.json index 09963394d3..adbd071178 100644 --- a/cspell.json +++ b/cspell.json @@ -254,6 +254,6 @@ "xmarw", "zkochan", "zoli", - "zoltan" + "zoltan" ] } diff --git a/exec/files-include-install-scripts/src/index.ts b/exec/files-include-install-scripts/src/index.ts index e824719b40..360eafb1f8 100644 --- a/exec/files-include-install-scripts/src/index.ts +++ b/exec/files-include-install-scripts/src/index.ts @@ -1,5 +1,4 @@ export function filesIncludeInstallScripts (filesIndex: Record): boolean { return filesIndex['binding.gyp'] != null || - filesIndex['/binding.gyp'] != null || - Object.keys(filesIndex).some((filename) => !(filename.match(/^[/]?[.]hooks[\\/]/) == null)) // TODO: optimize this + Object.keys(filesIndex).some((filename) => !(filename.match(/^[.]hooks[\\/]/) == null)) // TODO: optimize this } diff --git a/pkg-manager/core/src/getPeerDependencyIssues.ts b/pkg-manager/core/src/getPeerDependencyIssues.ts index b030ec0c7f..2daa8e4d00 100644 --- a/pkg-manager/core/src/getPeerDependencyIssues.ts +++ b/pkg-manager/core/src/getPeerDependencyIssues.ts @@ -17,7 +17,6 @@ export type ListMissingPeersOptions = Partial | 'preferWorkspacePackages' | 'saveWorkspaceProtocol' | 'storeController' -| 'useExperimentalNpmjsFilesIndex' | 'useGitBranchLockfile' | 'workspacePackages' > @@ -83,7 +82,6 @@ export async function getPeerDependencyIssues ( saveWorkspaceProtocol: false, // this doesn't matter in our case. We won't write changes to package.json files storeController: opts.storeController, tag: 'latest', - useExperimentalNpmjsFilesIndex: opts.useExperimentalNpmjsFilesIndex ?? false, virtualStoreDir: ctx.virtualStoreDir, wantedLockfile: ctx.wantedLockfile, workspacePackages: opts.workspacePackages ?? {}, diff --git a/pkg-manager/core/src/install/extendInstallOptions.ts b/pkg-manager/core/src/install/extendInstallOptions.ts index f0a55e4a51..aaca95f670 100644 --- a/pkg-manager/core/src/install/extendInstallOptions.ts +++ b/pkg-manager/core/src/install/extendInstallOptions.ts @@ -48,7 +48,6 @@ export interface StrictInstallOptions { lockfileIncludeTarballUrl: boolean preferWorkspacePackages: boolean preserveWorkspaceProtocol: boolean - useExperimentalNpmjsFilesIndex: boolean scriptsPrependNodePath: boolean | 'warn-only' scriptShell?: string shellEmulator: boolean @@ -200,7 +199,6 @@ const defaults = (opts: InstallOptions) => { resolutionMode: 'lowest-direct', saveWorkspaceProtocol: 'rolling', lockfileIncludeTarballUrl: false, - useExperimentalNpmjsFilesIndex: false, scriptsPrependNodePath: false, shamefullyHoist: false, shellEmulator: false, diff --git a/pkg-manager/core/src/install/index.ts b/pkg-manager/core/src/install/index.ts index 07e39b1df6..3c03a3749f 100644 --- a/pkg-manager/core/src/install/index.ts +++ b/pkg-manager/core/src/install/index.ts @@ -1036,7 +1036,6 @@ const _installInContext: InstallFunction = async (projects, ctx, opts) => { wantedLockfile: ctx.wantedLockfile, workspacePackages: opts.workspacePackages, patchedDependencies: opts.patchedDependencies, - useExperimentalNpmjsFilesIndex: opts.lockfileOnly && opts.useExperimentalNpmjsFilesIndex, lockfileIncludeTarballUrl: opts.lockfileIncludeTarballUrl, resolvePeersFromWorkspaceRoot: opts.resolvePeersFromWorkspaceRoot, supportedArchitectures: opts.supportedArchitectures, diff --git a/pkg-manager/core/test/install/lockfileOnly.ts b/pkg-manager/core/test/install/lockfileOnly.ts index f1532cd9a2..601e6ba51d 100644 --- a/pkg-manager/core/test/install/lockfileOnly.ts +++ b/pkg-manager/core/test/install/lockfileOnly.ts @@ -1,5 +1,4 @@ import path from 'path' -import { promises as fs } from 'fs' import { assertStore } from '@pnpm/assert-store' import { WANTED_LOCKFILE } from '@pnpm/constants' import { prepareEmpty } from '@pnpm/prepare' @@ -90,12 +89,3 @@ test('do not update the lockfile when lockfileOnly and frozenLockfile are both u frozenLockfile: true, }))).rejects.toThrow(/is not up to date/) }) - -test('a lockfile only update with the useExperimentalNpmjsFilesIndex flag resolves without packages being fetched', async () => { - const project = prepareEmpty() - const opts = await testDefaults({ useExperimentalNpmjsFilesIndex: true, lockfileOnly: true }) - await addDependenciesToPackage({}, ['nodecv@1.1.2'], opts) - const lockfile = await project.readLockfile() - expect(lockfile.packages!['/nodecv@1.1.2'].requiresBuild).toBeTruthy() - expect(await fs.readdir(opts.storeDir)).toStrictEqual([]) -}) diff --git a/pkg-manager/plugin-commands-installation/src/install.ts b/pkg-manager/plugin-commands-installation/src/install.ts index ed41fa961b..fa5f459b11 100644 --- a/pkg-manager/plugin-commands-installation/src/install.ts +++ b/pkg-manager/plugin-commands-installation/src/install.ts @@ -270,7 +270,6 @@ export type InstallCommandOptions = Pick & { diff --git a/pkg-manager/resolve-dependencies/package.json b/pkg-manager/resolve-dependencies/package.json index 3ccb2d8de6..f599513a35 100644 --- a/pkg-manager/resolve-dependencies/package.json +++ b/pkg-manager/resolve-dependencies/package.json @@ -33,7 +33,6 @@ "@pnpm/core-loggers": "workspace:*", "@pnpm/dependency-path": "workspace:*", "@pnpm/error": "workspace:*", - "@pnpm/fetch": "workspace:*", "@pnpm/exec.files-include-install-scripts": "workspace:*", "@pnpm/lockfile-types": "workspace:*", "@pnpm/lockfile-utils": "workspace:*", diff --git a/pkg-manager/resolve-dependencies/src/checkViaRegistryIfPkgRequiresBuild.ts b/pkg-manager/resolve-dependencies/src/checkViaRegistryIfPkgRequiresBuild.ts deleted file mode 100644 index 992f28c32a..0000000000 --- a/pkg-manager/resolve-dependencies/src/checkViaRegistryIfPkgRequiresBuild.ts +++ /dev/null @@ -1,68 +0,0 @@ -import { PnpmError } from '@pnpm/error' -import { filesIncludeInstallScripts } from '@pnpm/exec.files-include-install-scripts' -import { fetch } from '@pnpm/fetch' -import { type ProjectManifest } from '@pnpm/types' - -interface RegistryFileFields { - size: number - type: 'File' - path: string - contentType: string - hex: string - isBinary: boolean - linesCount: number -} - -interface RegistryFileList { - files: { - [path: string]: RegistryFileFields - } -} - -export async function checkViaRegistryIfPkgRequiresBuild (pkgName: string, pkgVersion: string): Promise { - try { - const regFS = await fetchPkgIndex(pkgName, pkgVersion) - const hasInstall = filesIncludeInstallScripts(regFS.files) - if (hasInstall) return true - - const pkgJsonHex = regFS.files['/package.json'].hex - const pkgJsonContent = await fetchSpecificFileFromRegistryFS(pkgName, pkgVersion, pkgJsonHex) - const pkgJson = JSON.parse(pkgJsonContent) as ProjectManifest - return pkgManifestHasInstallScripts(pkgJson) - } catch (err) { - if (err instanceof Error) { - throw new PnpmError('REG_FS_PARSE', `Failed to fetch ${pkgName}@${pkgVersion}: ${err.message}`) - } - } - return false -} - -async function fetchPkgIndex (pkgName: string, pkgVersion: string): Promise { - const url = `https://npmjs.com/package/${pkgName}/v/${pkgVersion}/index` - const fetchResult = await fetch(url) - if (!fetchResult.ok) { - throw new PnpmError('PKG_INDEX_FETCH', `Failed to fetch ${pkgName}@${pkgVersion} Status: ${fetchResult.statusText}`) - } - - const fetchJSON = await fetchResult.json() as RegistryFileList - if (fetchJSON.files == null) { - throw new PnpmError('PKG_INDEX_FILE_PARSE', `Unable to parse file list for ${pkgName}@${pkgVersion} Status: ${fetchResult.statusText}`) - } - return fetchJSON -} - -async function fetchSpecificFileFromRegistryFS (pkgName: string, pkgVersion: string, fileHex: string): Promise { - const url = `https://npmjs.com/package/${pkgName}/file/${fileHex}` - const fetchResult = await fetch(url) - if (!fetchResult.ok) { - throw new PnpmError('REG_FS_ERROR', `Failed to fetch ${pkgName}@${pkgVersion}, file: ${fileHex} Status: ${fetchResult.statusText}`) - } - return fetchResult.text() -} - -export function pkgManifestHasInstallScripts (manifest: ProjectManifest | undefined): boolean { - if (!manifest?.scripts) return false - return Boolean(manifest.scripts.preinstall) || - Boolean(manifest.scripts.install) || - Boolean(manifest.scripts.postinstall) -} diff --git a/pkg-manager/resolve-dependencies/src/index.ts b/pkg-manager/resolve-dependencies/src/index.ts index f4bea5f9cc..b3dd3b6217 100644 --- a/pkg-manager/resolve-dependencies/src/index.ts +++ b/pkg-manager/resolve-dependencies/src/index.ts @@ -26,7 +26,6 @@ import promiseShare from 'promise-share' import difference from 'ramda/src/difference' import zipWith from 'ramda/src/zipWith' import isSubdir from 'is-subdir' -import { checkViaRegistryIfPkgRequiresBuild, pkgManifestHasInstallScripts } from './checkViaRegistryIfPkgRequiresBuild' import { getWantedDependencies, type WantedDependency } from './getWantedDependencies' import { depPathToRef } from './depPathToRef' import { createNodeIdForLinkedLocalPkg, type UpdateMatchingFunction } from './resolveDependencies' @@ -101,7 +100,6 @@ export async function resolveDependencies ( saveWorkspaceProtocol: 'rolling' | boolean lockfileIncludeTarballUrl?: boolean allowNonAppliedPatches?: boolean - useExperimentalNpmjsFilesIndex?: boolean } ) { const _toResolveImporter = toResolveImporter.bind(null, { @@ -288,7 +286,7 @@ export async function resolveDependencies ( return { dependenciesByProjectId, dependenciesGraph, - finishLockfileUpdates: promiseShare(finishLockfileUpdates(dependenciesGraph, pendingRequiresBuilds, newLockfile, opts.useExperimentalNpmjsFilesIndex)), + finishLockfileUpdates: promiseShare(finishLockfileUpdates(dependenciesGraph, pendingRequiresBuilds, newLockfile)), outdatedDependencies, linkedDependenciesByProjectId, newLockfile, @@ -324,8 +322,7 @@ function verifyPatches ( async function finishLockfileUpdates ( dependenciesGraph: DependenciesGraph, pendingRequiresBuilds: string[], - newLockfile: Lockfile, - useExperimentalNpmjsFilesIndex: boolean + newLockfile: Lockfile ) { return Promise.all(pendingRequiresBuilds.map(async (depPath) => { const depNode = dependenciesGraph[depPath] @@ -336,12 +333,17 @@ async function finishLockfileUpdates ( // We assume that all optional dependencies have to be built. // Optional dependencies are not always downloaded, so there is no way to know whether they need to be built or not. requiresBuild = true - } else if (useExperimentalNpmjsFilesIndex) { - requiresBuild = await checkViaRegistryIfPkgRequiresBuild(depNode.name, depNode.version) } else { // The npm team suggests to always read the package.json for deciding whether the package has lifecycle scripts const { files, bundledManifest: pkgJson } = await depNode.fetching() - requiresBuild = pkgManifestHasInstallScripts(pkgJson) || filesIncludeInstallScripts(files.filesIndex) + requiresBuild = Boolean( + pkgJson?.scripts != null && ( + Boolean(pkgJson.scripts.preinstall) || + Boolean(pkgJson.scripts.install) || + Boolean(pkgJson.scripts.postinstall) + ) || + filesIncludeInstallScripts(files.filesIndex) + ) } if (typeof depNode.requiresBuild === 'function') { depNode.requiresBuild['resolve'](requiresBuild) diff --git a/pkg-manager/resolve-dependencies/src/resolveDependencies.ts b/pkg-manager/resolve-dependencies/src/resolveDependencies.ts index 8850faab3a..5f6b48743e 100644 --- a/pkg-manager/resolve-dependencies/src/resolveDependencies.ts +++ b/pkg-manager/resolve-dependencies/src/resolveDependencies.ts @@ -158,7 +158,6 @@ export interface ResolutionContext { pnpmVersion: string registries: Registries resolutionMode?: 'highest' | 'time-based' | 'lowest-direct' - useExperimentalNpmjsFilesIndex: boolean virtualStoreDir: string workspacePackages?: WorkspacePackages missingPeersOfChildrenByPkgId: Record @@ -1087,8 +1086,9 @@ async function resolveDependency ( ? ctx.lockfileDir : options.parentPkg.rootDir, registry: wantedDependency.alias && pickRegistryForPackage(ctx.registries, wantedDependency.alias, wantedDependency.pref) || ctx.registries.default, - // we can skip fetching only if we use the NPMJS Files Index, and we don't need the full tarball - skipFetch: !!ctx.useExperimentalNpmjsFilesIndex, + // Unfortunately, even when run with --lockfile-only, we need the *real* package.json + // so fetching of the tarball cannot be ever avoided. Related issue: https://github.com/pnpm/pnpm/issues/1176 + skipFetch: false, update: options.update, workspacePackages: ctx.workspacePackages, supportedArchitectures: options.supportedArchitectures, @@ -1247,6 +1247,7 @@ async function resolveDependency ( pkg.deprecated = currentPkg.dependencyLockfile.deprecated } hasBin = Boolean((pkg.bin && !(pkg.bin === '' || Object.keys(pkg.bin).length === 0)) ?? pkg.directories?.bin) + /* eslint-enable @typescript-eslint/dot-notation */ } if (options.currentDepth === 0 && pkgResponse.body.latest && pkgResponse.body.latest !== pkg.version) { ctx.outdatedDependencies[pkgResponse.body.id] = pkgResponse.body.latest diff --git a/pkg-manager/resolve-dependencies/src/resolveDependencyTree.ts b/pkg-manager/resolve-dependencies/src/resolveDependencyTree.ts index fa0b251447..3981c52d13 100644 --- a/pkg-manager/resolve-dependencies/src/resolveDependencyTree.ts +++ b/pkg-manager/resolve-dependencies/src/resolveDependencyTree.ts @@ -85,7 +85,6 @@ export interface ResolveDependenciesOptions { lockfileDir: string storeController: StoreController tag: string - useExperimentalNpmjsFilesIndex: boolean virtualStoreDir: string wantedLockfile: Lockfile workspacePackages: WorkspacePackages @@ -124,7 +123,6 @@ export async function resolveDependencyTree ( resolutionMode: opts.resolutionMode, skipped: wantedToBeSkippedPackageIds, storeController: opts.storeController, - useExperimentalNpmjsFilesIndex: opts.useExperimentalNpmjsFilesIndex, virtualStoreDir: opts.virtualStoreDir, wantedLockfile: opts.wantedLockfile, appliedPatches: new Set(), diff --git a/pkg-manager/resolve-dependencies/tsconfig.json b/pkg-manager/resolve-dependencies/tsconfig.json index 40abdc421b..03229e3b10 100644 --- a/pkg-manager/resolve-dependencies/tsconfig.json +++ b/pkg-manager/resolve-dependencies/tsconfig.json @@ -27,9 +27,6 @@ { "path": "../../lockfile/prune-lockfile" }, - { - "path": "../../network/fetch" - }, { "path": "../../packages/constants" }, diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 38d24defc6..4d974a0416 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -4065,9 +4065,6 @@ importers: '@pnpm/error': specifier: workspace:* version: link:../../packages/error - '@pnpm/fetch': - specifier: workspace:* - version: link:../../network/fetch '@pnpm/exec.files-include-install-scripts': specifier: workspace:* version: link:../../exec/files-include-install-scripts