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

feat(deploy): apply publishConfig to all packages during deploy #6943

Merged
merged 4 commits into from
Aug 24, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/flat-kids-jump.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@pnpm/plugin-commands-deploy": minor
"@pnpm/directory-fetcher": minor
---

Apply `publishConfig` for workspace packages on directory fetch. Enables a publishable ("exportable") `package.json` on deployment [#6693](https://github.com/pnpm/pnpm/issues/6693).
4 changes: 4 additions & 0 deletions fetching/directory-fetcher/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,13 @@
"@pnpm/logger": "^5.0.0"
},
"dependencies": {
"@pnpm/exportable-manifest": "workspace:*",
"@pnpm/fetcher-base": "workspace:*",
"@pnpm/read-project-manifest": "workspace:*",
"@pnpm/resolver-base": "workspace:*",
"@pnpm/types": "workspace:*",
"@pnpm/write-project-manifest": "workspace:*",
"fast-deep-equal": "^3.1.3",
"npm-packlist": "^5.1.3"
},
"devDependencies": {
Expand Down
31 changes: 23 additions & 8 deletions fetching/directory-fetcher/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
import { promises as fs, type Stats } from 'fs'
import path from 'path'
import { createExportableManifest } from '@pnpm/exportable-manifest'
import type { DirectoryFetcher, DirectoryFetcherOptions } from '@pnpm/fetcher-base'
import { logger } from '@pnpm/logger'
import { safeReadProjectManifestOnly } from '@pnpm/read-project-manifest'
import type { ProjectManifest } from '@pnpm/types'
import { writeProjectManifest } from '@pnpm/write-project-manifest'
import equal from 'fast-deep-equal'
import packlist from 'npm-packlist'

const directoryFetcherLogger = logger('directory-fetcher')
Expand Down Expand Up @@ -47,11 +51,8 @@ async function fetchAllFilesFromDir (
opts: FetchFromDirOpts
) {
const filesIndex = await _fetchAllFilesFromDir(readFileStat, dir)
const manifest = await safeReadProjectManifestAndMakeExportable(dir, filesIndex) ?? {}
if (opts.manifest) {
// In a regular pnpm workspace it will probably never happen that a dependency has no package.json file.
// Safe read was added to support the Bit workspace in which the components have no package.json files.
// Related PR in Bit: https://github.com/teambit/bit/pull/5251
const manifest = await safeReadProjectManifestOnly(dir) ?? {}
opts.manifest.resolve(manifest as any) // eslint-disable-line @typescript-eslint/no-explicit-any
}
return {
Expand Down Expand Up @@ -128,11 +129,8 @@ async function fetchPackageFilesFromDir (
) {
const files = await packlist({ path: dir })
const filesIndex: Record<string, string> = Object.fromEntries(files.map((file) => [file, path.join(dir, file)]))
const manifest = await safeReadProjectManifestAndMakeExportable(dir, filesIndex) ?? {}
if (opts.manifest) {
// In a regular pnpm workspace it will probably never happen that a dependency has no package.json file.
// Safe read was added to support the Bit workspace in which the components have no package.json files.
// Related PR in Bit: https://github.com/teambit/bit/pull/5251
const manifest = await safeReadProjectManifestOnly(dir) ?? {}
opts.manifest.resolve(manifest as any) // eslint-disable-line @typescript-eslint/no-explicit-any
}
return {
Expand All @@ -141,3 +139,20 @@ async function fetchPackageFilesFromDir (
packageImportMethod: 'hardlink' as const,
}
}

async function safeReadProjectManifestAndMakeExportable (
dir: string,
filesIndex: Record<string, string>
): Promise<ProjectManifest | null> {
const manifest = await safeReadProjectManifestOnly(dir)
// In a regular pnpm workspace it will probably never happen that a dependency has no package.json file.
// Safe read was added to support the Bit workspace in which the components have no package.json files.
// Related PR in Bit: https://github.com/teambit/bit/pull/5251
if (!manifest) return null
const exportableManifest = await createExportableManifest(dir, manifest)
if (equal(manifest, exportableManifest)) return manifest
const manifestPathOverride = path.join(dir, 'node_modules/.pnpm/package.json')
await writeProjectManifest(manifestPathOverride, exportableManifest)
filesIndex['package.json'] = manifestPathOverride
return manifest
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"name": "exportable",
"version": "1.0.0",
"main": "src/index.ts",
"publishConfig": {
"main": "dist/index.js"
}
}
21 changes: 21 additions & 0 deletions fetching/directory-fetcher/test/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -157,3 +157,24 @@ describe('fetch resolves symlinked files to their real locations', () => {
expect(fetchResult.filesIndex['src/index.js']).toBe(path.resolve('src/index.js'))
})
})

test('fetch should return exportable manifest', async () => {
process.chdir(f.find('exportable-manifest'))
const fetcher = createDirectoryFetcher()

// eslint-disable-next-line
const fetchResult = await fetcher.directory({} as any, {
directory: '.',
type: 'directory',
}, {
lockfileDir: process.cwd(),
})

expect(fetchResult.filesIndex['package.json']).not.toBe(path.resolve('package.json'))

expect(JSON.parse(fs.readFileSync(fetchResult.filesIndex['package.json'], 'utf8'))).toStrictEqual({
name: 'exportable',
version: '1.0.0',
main: 'dist/index.js',
})
})
9 changes: 9 additions & 0 deletions fetching/directory-fetcher/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,18 @@
{
"path": "../../__utils__/test-fixtures"
},
{
"path": "../../packages/types"
},
{
"path": "../../pkg-manifest/exportable-manifest"
},
{
"path": "../../pkg-manifest/read-project-manifest"
},
{
"path": "../../pkg-manifest/write-project-manifest"
},
{
"path": "../../resolving/resolver-base"
},
Expand Down
24 changes: 24 additions & 0 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 5 additions & 1 deletion releasing/plugin-commands-deploy/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,11 @@
"@pnpm/lockfile-types": "workspace:*",
"@pnpm/plugin-commands-deploy": "workspace:*",
"@pnpm/prepare": "workspace:*",
"@pnpm/registry-mock": "3.11.0"
"@pnpm/registry-mock": "3.11.0",
"@types/cross-spawn": "^6.0.2",
"cross-spawn": "^7.0.3",
"load-json-file": "^6.2.0",
"write-yaml-file": "^5.0.0"
},
"dependencies": {
"@pnpm/cli-utils": "workspace:*",
Expand Down
38 changes: 38 additions & 0 deletions releasing/plugin-commands-deploy/test/deploy.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,15 @@ import path from 'path'
import { deploy } from '@pnpm/plugin-commands-deploy'
import { assertProject } from '@pnpm/assert-project'
import { preparePackages } from '@pnpm/prepare'
import { REGISTRY_MOCK_PORT } from '@pnpm/registry-mock'
import { readProjects } from '@pnpm/filter-workspace-packages'
import crossSpawn from 'cross-spawn'
import { sync as loadJsonFile } from 'load-json-file'
import writeYamlFile from 'write-yaml-file'
import { DEFAULT_OPTS } from './utils'

const pnpmBin = path.join(__dirname, '../../../pnpm/bin/pnpm.cjs')

test('deploy', async () => {
preparePackages([
{
Expand All @@ -20,6 +26,10 @@ test('deploy', async () => {
'project-3': 'workspace:*',
'is-negative': '1.0.0',
},
main: 'local-file-1.js',
publishConfig: {
main: 'publish-file-1.js',
},
},
{
name: 'project-2',
Expand All @@ -29,6 +39,10 @@ test('deploy', async () => {
'project-3': 'workspace:*',
'is-odd': '1.0.0',
},
main: 'local-file-2.js',
publishConfig: {
main: 'publish-file-2.js',
},
},
{
name: 'project-3',
Expand All @@ -46,6 +60,10 @@ test('deploy', async () => {
fs.writeFileSync(`${name}/index.js`, '', 'utf8')
})

await writeYamlFile('pnpm-workspace.yaml', { packages: ['*'] })
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to createExportableManifest we need metadata only available after an install.

Presently, deploy does not actually require an install previously.

This could be considered a breaking change. Although I would assume most users already have the relevant packages installed, and expected this behavior. This is inline with how commands like publish work.

deploy could be considered a "local" publish.

crossSpawn.sync(pnpmBin, ['install', '--ignore-scripts', '--store-dir=../store', `--registry=http://localhost:${REGISTRY_MOCK_PORT}`])
fs.rmSync('pnpm-lock.yaml')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A little bit of a hack for testing purposes... an assertion below checks that this file does not exist (as a side effect of the install called internally to deploy) and wanted to maintain that assertion.


const { allProjects, selectedProjectsGraph } = await readProjects(process.cwd(), [{ namePattern: 'project-1' }])

await deploy.handler({
Expand Down Expand Up @@ -73,6 +91,26 @@ test('deploy', async () => {
expect(fs.existsSync('deploy/node_modules/.pnpm/file+project-3/node_modules/project-3/index.js')).toBeTruthy()
expect(fs.existsSync('deploy/node_modules/.pnpm/file+project-3/node_modules/project-3/test.js')).toBeFalsy()
expect(fs.existsSync('pnpm-lock.yaml')).toBeFalsy() // no changes to the lockfile are written
const project1Manifest = loadJsonFile('deploy/package.json')
expect(project1Manifest).toMatchObject({
name: 'project-1',
main: 'publish-file-1.js',
dependencies: {
'is-positive': '1.0.0',
'project-2': '2.0.0',
},
})
expect(project1Manifest).not.toHaveProperty('publishConfig')
const project2Manifest = loadJsonFile('deploy/node_modules/.pnpm/file+project-2/node_modules/project-2/package.json')
expect(project2Manifest).toMatchObject({
name: 'project-2',
main: 'publish-file-2.js',
dependencies: {
'project-3': '2.0.0',
'is-odd': '1.0.0',
},
})
expect(project2Manifest).not.toHaveProperty('publishConfig')
})

test('deploy with dedupePeerDependents=true ignores the value of dedupePeerDependents', async () => {
Expand Down