tiktok / pnpm-sync Goto Github PK

For integration with Rush, best practice with .pnpm-sync.json files would be to read them all up front before execution starts, validate that the files have the expected versions, and then error out early.

This helps clean up the logging output by reporting configuration errors with .pnpm-sync at the start of the process, rather than partway through a build. In the current configuration, the logging output can get buried when there are a large number of projects, e.g.

==[ FAILURE: 2 operations ]====================================================

--[ FAILURE: @rushstack/eslint-patch (build) ]--------------[ 14.38 seconds ]--


--[ FAILURE: @rushstack/tree-pattern (build) ]---------------[ 2.98 seconds ]--



Operations failed.

rush build (14.53 seconds)

currently get this error:

❯ npx pnpm-sync
node_modules/.bin/pnpm-sync: 1: Syntax error: word unexpected (expecting ")")

From https://github.com/microsoft/rushstack/pull/4530/files#r1504932679:

2. Also I would suggest to improve the formatting of the message:

   pnpm-sync: Copied 107 files in 95ms from C:\Git\rushstack\libraries\terminal
   

3. It seems pnpmSyncCopy() is printing this message directly to the console. We should improve the API to provide a messaging callback that allows the output to be redirected and (ideally) optionally reformatted. Something like this:

   pnpmSyncCopy({
     pnpmSyncJsonPath,
     messageCallback: ({ message, messageKind: 'error'|'warning'|'info'|'verbose'|'timing', messageId, details }) => {
       if (messageKind === 'verbose' && !debug) {
         return;
       }
       switch (messageId) {
         case 'sync-finished':
           // customized logging; the structure of details can depend on messageId
           terminal.writeLine(colors.green('pnpm-sync') 
             + ` copied ${details.fileCount} files in ${details.totalMs} ms from ${details.sourcePath}`);
           break;
         default:
           // simple preformatted logging
           if (messageKind === 'error' | messageKind === 'warning') {
             console.error(message);
           } else {
             console.log(message);
           }
           break;
       }
     }
   })

   This would ensure that Rush (and in the future PNPM) has full ownership of its CLI UX.

The .pnpm-sync.json file should contain a version field so it can be invalidated and regenerated if its schema changes between versions of the pnpm-sync-lib packages, or if an issue is identified and fixed.

I'm seeing an issue on my current local clone of https://github.com/microsoft/rushstack, where the build-tests-subspace/typescript-v4-test/node_modules/@rushstack/heft folder is linked to the wrong location (see screenshot below) because the sourceFolder property was generated with the wrong value.

The .pnpm-sync.json file should have been regenerated after the bug creating the incorrect path was fixed, and that would have been easily identified if it contained a version field.

apps/heft/node_modules/.pnpm-sync.json:

{
  "postbuildInjectedCopy": {
    "sourceFolder": "../..",
    "targetFolders": [
      {
        "folderPath": "../../../../common/temp/node_modules/.pnpm/file+..+..+apps+heft/node_modules/@rushstack/heft"
      },
      {
        "folderPath": "../../../common/temp/build-tests-subspace/node_modules/.pnpm/file+..+..+..+apps+heft_@[email protected]/node_modules/@rushstack/heft"
      }
    ]
  }
}

build-tests-subspace/typescript-v4-test/node_modules/@rushstack/heft structure:

Existence checking a file before reading it (especially asynchronously) allows for the existence of the file to change between the two file system calls. Additionally, for integration with Rush multi-phase builds, the sync file contents will not change between phases in the same project, and thus by allowing the content to be passed directly, we can avoid duplicate reads when pnpm-sync will be called after each phase.

As an example, the rushstack repository has phases build and test for all projects.