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

JacobLey · 2023-08-15T21:40:37Z

When deploying packages, the package.json of the deployed package (as well as any other locally defined dependencies) should be treated as if it published, and mutate the package.json according to publishConfig and local workspace: dependencies.

Issue: #6693

welcome · 2023-08-15T21:40:43Z

💖 Thanks for opening this pull request! 💖
Please be patient and we will get back to you as soon as we can.

JacobLey · 2023-08-15T21:43:40Z

releasing/plugin-commands-deploy/test/deploy.test.ts

@@ -46,6 +59,10 @@ test('deploy', async () => {
    fs.writeFileSync(`${name}/index.js`, '', 'utf8')
  })

+  await writeYamlFile('pnpm-workspace.yaml', { packages: ['*'] })


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.

JacobLey · 2023-08-15T21:44:52Z

releasing/plugin-commands-deploy/test/deploy.test.ts

@@ -46,6 +59,10 @@ test('deploy', async () => {
    fs.writeFileSync(`${name}/index.js`, '', 'utf8')
  })

+  await writeYamlFile('pnpm-workspace.yaml', { packages: ['*'] })
+  crossSpawn.sync(pnpmBin, ['install', '--ignore-scripts', '--store-dir=../store', `--registry=http://localhost:${REGISTRY_MOCK_PORT}`])
+  fs.rmSync('pnpm-lock.yaml')


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.

zkochan · 2023-08-15T23:08:17Z

fs/indexed-pkg-importer/src/importIndexedDir.ts

+    if (opts.makeExportableManifest) {
+      const manifest = await readProjectManifest(newDir)
+      const exportableManifest = await createExportableManifest(
+        path.dirname(filenames[manifest.fileName]),
+        manifest.manifest
+      )
+      await manifest.writeProjectManifest(exportableManifest)
+    }


This is going to be used only by the deploy command, so I don't see any reason to put this code to importIndexedDir. Just place it in deploy.ts

Updated to put all createExportableManifest logic in deploy.ts.

Removed all changes to other packages (adding createExportableManifest option)

JacobLey · 2023-08-16T15:25:15Z

Build is currently only failing on ubuntu+v16 on this test: https://github.com/pnpm/pnpm/blob/main/pnpm/test/filterProd.test.ts#L9

This PR does not touch filtering, install, or recursive execution, so I would be surprised if I explicitly broke something.

Will defer to maintainer for guidance if there is something I can do to fix? Or perhaps just retry the test (flaky)?

zkochan · 2023-08-16T23:19:32Z

The issue with this solution is that now createExportableManifest will be executed on all the dependencies. Even though we only need to run it on dependencies linked from local directories.

I suggest a different solution. Make the change in directory-fetcher. It create an index file that is passed to the linker. You can write the "exportable manifest" into node_modules/.pnpm/package.json and provide the path to it to the files index map. This way the package.json file that you create will be linked, not the original one.

JacobLey · 2023-08-17T03:16:44Z

will be executed on all the dependencies

Agreed, should only be executed on local packages.

Looking into directory-fetcher, but we are back to the original issue that we will need to start passing some flag around to other packages for this single use case. Which I am ok implementing, but calling it out.

Another possible solution is to look up the list of all packages in the workspace pre-install (via something like findWorkspacePackages). Then reference that list for each hook and check "is this package a local workspace package". If true -> createExportableManifest.
There may even be an existing utility that does that check?

write the "exportable manifest" into node_modules/.pnpm/package.json

That sounds safe to me. Do we want to clean up the file after we are done with it?
Such as

process.on('exit', () => fs.rm('node_modules/.pnpm/package.json')

zkochan · 2023-08-17T12:39:01Z

Looking into directory-fetcher, but we are back to the original issue that we will need to start passing some flag around to other packages for this single use case. Which I am ok implementing, but calling it out.

No, I think we can make this the default behaviour for all local directory dependencies. No need to pass options.

JacobLey · 2023-08-17T21:22:25Z

Updated, but not ready to merge.

~~Tests are now (rightfully) failing on PnP recursion https://github.com/pnpm/pnpm/blob/main/exec/plugin-commands-script-runners/test/exec.e2e.ts#L487~~

~~Not entirely sure how I broke that, but there must be some expectation that the package.json link remains unaltered.~~
Looks like this is related to my local setup somehow. Not seeing similar failure in CI build.

JacobLey · 2023-08-17T21:29:15Z

pkg-manifest/read-project-manifest/src/index.ts

-type WriteProjectManifest = (manifest: ProjectManifest, force?: boolean) => Promise<void>
+interface WriteProjectManifestOptions {
+  force?: boolean
+  manifestPathOverride?: string


The existing behavior was to re-write the file in same location it was loaded from. That is unwanted for our use case, so provided this override option.

This "breaks" existing options syntax, although I did not find any usage of the force option... so actually worked in-repo.

If this method is considered part of pnpm's public API though, I can tweak to accept a boolean or this options object for backwards compatibility.

Alternatively, I can use the writeProjectManifest function directly in directory-fetcher, but I did appreciate that the manifest.writeProjectManifest came with sensible defaults

Don't add this option. Just use a function that accepts the path to the manifest for saving the manifest.

JacobLey · 2023-08-17T21:30:36Z

fetching/directory-fetcher/src/index.ts

+    filesIndex[fileName] = manifestPathOverride
+  }
+
+  return manifest?.manifest ?? null


Not sure exactly what behavior depends on it, but would it be preferable to return exportableManifest here instead?

FWIW that did not solve my current failing tests anyways.

JacobLey · 2023-08-17T21:32:30Z

fetching/directory-fetcher/src/index.ts

+      manifest.manifest
+    )
+    const manifestPathOverride = path.join(dir, 'node_modules', '.pnpm', fileName)
+    await manifest.writeProjectManifest(exportableManifest, { manifestPathOverride })


Here is the use case of manifestPathOverride. As mentioned in other comment, could replace this call with raw writeProjectManifest (rather than the one built into the safeRead response)

zkochan · 2023-08-17T23:44:37Z

fetching/directory-fetcher/src/index.ts

+
+  if (manifest) {
+    const { fileName } = manifest
+    const exportableManifest = await createExportableManifest(


Let's actually check if the exportableManifest differs from the original one. If it doesn't differ, there's no. need to write it to the file system and the path to the original file may be used.

zkochan · 2023-08-17T23:48:24Z

pkg-manifest/read-project-manifest/src/index.ts

-type WriteProjectManifest = (manifest: ProjectManifest, force?: boolean) => Promise<void>
+interface WriteProjectManifestOptions {
+  force?: boolean
+  manifestPathOverride?: string


Don't add this option. Just use a function that accepts the path to the manifest for saving the manifest.

When deploying packages, the package.json of the deployed package (as well as any other locally defined dependencies) should be treated as if it published, and mutate the package.json according to `publishConfig` and local `workspace:` dependencies. Issue: pnpm#6693

JacobLey · 2023-08-18T04:51:32Z

Updated to use separate write function, so doesn't alter existing parameters.
Also check for diff before writing, to hopefully optimize file write.

One (trivial) call out is that the package will not necessarily be formatted the same way as before. Which I guess wasn't ever a guarantee (e.g. multi-newline in JSON file is removed) but now we would also lose comments in a JSON5, or change the indent settings.

welcome · 2023-08-24T23:13:32Z

Congrats on merging your first pull request! 🎉🎉🎉

…loy (#6943)" This reverts commit d57e4de.

…loy (#6943)" (#7058) This reverts commit d57e4de. This reverts #6943 This will solve: - #7040 - #6994

When deploying packages, the package.json of the deployed package (as well as any other locally defined dependencies) should be treated as if it published, and mutate the package.json according to `publishConfig` and local `workspace:` dependencies. close #6693 --------- Co-authored-by: Zoltan Kochan <z@kochan.io>

JacobLey requested a review from zkochan as a code owner August 15, 2023 21:40

JacobLey commented Aug 15, 2023

View reviewed changes

zkochan requested changes Aug 15, 2023

View reviewed changes

JacobLey force-pushed the publishConfigDeploy branch from 4049b5c to 6e980d0 Compare August 16, 2023 02:35

JacobLey requested a review from zkochan August 16, 2023 02:38

JacobLey force-pushed the publishConfigDeploy branch from 6e980d0 to 0729698 Compare August 17, 2023 21:20

JacobLey commented Aug 17, 2023

View reviewed changes

zkochan reviewed Aug 17, 2023

View reviewed changes

JacobLey force-pushed the publishConfigDeploy branch from 0729698 to b12b3e9 Compare August 18, 2023 04:38

JacobLey requested a review from zkochan August 18, 2023 04:45

zkochan added 3 commits August 25, 2023 00:54

Merge remote-tracking branch 'origin/main' into publishConfigDeploy

c55f849

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

474d2c4

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

24b699f

zkochan approved these changes Aug 24, 2023

View reviewed changes

zkochan merged commit d57e4de into pnpm:main Aug 24, 2023
3 of 8 checks passed

JacobLey mentioned this pull request Aug 28, 2023

pnpm deploy doesn't rewrite workspace:* dependencies in package.json #6269

Open

zkochan mentioned this pull request Aug 29, 2023

8.7.0 regression, failed to run deploy command with workspace protocol dependencies #6994

Closed

4 tasks

This was referenced Sep 4, 2023

8.7.0 regression: Install of monorepo that contains locally injected packages fails #7040

Closed

chore(install): add test for injected installs #7048

Closed

zkochan added a commit that referenced this pull request Sep 5, 2023

revert: "feat(deploy): apply publishConfig to all packages during dep…

4dabc31

…loy (#6943)" This reverts commit d57e4de.

zkochan mentioned this pull request Sep 5, 2023

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

Merged

zkochan added a commit that referenced this pull request Sep 5, 2023

revert: "feat(deploy): apply publishConfig to all packages during dep…

d920708

…loy (#6943)" (#7058) This reverts commit d57e4de. This reverts #6943 This will solve: - #7040 - #6994

zkochan mentioned this pull request Feb 13, 2024

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

Draft

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

JacobLey commented Aug 15, 2023

welcome bot commented Aug 15, 2023

JacobLey Aug 15, 2023

JacobLey Aug 15, 2023

zkochan Aug 15, 2023

JacobLey Aug 16, 2023

JacobLey commented Aug 16, 2023

zkochan commented Aug 16, 2023

JacobLey commented Aug 17, 2023

zkochan commented Aug 17, 2023

JacobLey commented Aug 17, 2023 •

edited

JacobLey Aug 17, 2023

zkochan Aug 17, 2023

JacobLey Aug 17, 2023

JacobLey Aug 17, 2023

zkochan Aug 17, 2023

zkochan Aug 17, 2023

JacobLey commented Aug 18, 2023

welcome bot commented Aug 24, 2023

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

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

Conversation

JacobLey commented Aug 15, 2023

welcome bot commented Aug 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JacobLey commented Aug 16, 2023

zkochan commented Aug 16, 2023

JacobLey commented Aug 17, 2023

zkochan commented Aug 17, 2023

JacobLey commented Aug 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JacobLey commented Aug 18, 2023

welcome bot commented Aug 24, 2023

JacobLey commented Aug 17, 2023 •

edited