Fix relative link paths in pnpm lockfile

[0916dhkim]

Problem Breakdown

Right now, if you have a monorepo setup with an internal dependency depending on another internal package, the generated pnpm-lock.yaml file does not resolve the paths correctly.

Let's say we have a dependency tree like this.
@package/to-be-isolated (located at packages/to-be-isolated) -> @package/first (located at packages/first) -> @package/second (located at packages/second)
The expected pnpm-lock.yaml content is

importers:

  .:
    dependencies:
      '@package/first':
        specifier: workspace:*
        version: link:./packages/first

  packages/first:
    dependencies:
      '@package/second':
        specifier: workspace:*
        version: link:../second

  packages/second: {}

However, the actual output is

importers:

  .:
    dependencies:
      '@package/first':
        specifier: workspace:*
        version: link:./packages/first

  packages/first:
    dependencies:
      '@package/second':
        specifier: workspace:*
        version: link:./packages/second # <-THIS IS INCORRECT

  packages/second: {}

[0916dhkim]

Because directoryByPackageName[key] is a path relative to the project root, I think we need to add a logic to resolve a relative path from the importer's path.

[0x80]

I don't quite understand why this check would be necessary:

if (!relativePath.startsWith(".") && !relativePath.startsWith("/"))

1. In what scenario does path.relative() return a valid path that is not starting with "."?
2. What valid relative path would start with a "/"?

[0916dhkim]

1. path.relative formats the path like "dir1/dir2" instead of "./dir1/dir2". I saw pnpm-lock.yaml file has all path starting with "./" so I added this line.
2. Good point! I can't think of any case.

[0x80]

I've tested it and it seems to work for me. If you can simplify the check I can merge this. I suggest to change the function to:

function pnpmMapDependenciesLinks(
  importerPath: string,
  def: ResolvedDependencies,
  directoryByPackageName: { [packageName: string]: string }
): ResolvedDependencies {
  return mapValues(def, (value, key) => {
    if (!value.startsWith("link:")) {
      return value;
    }

    const relativePath = path.relative(
      importerPath,
      directoryByPackageName[key]
    );

    return relativePath.startsWith(".")
      ? `link:${relativePath}`
      : `link:./${relativePath}`;
  });
}

[0916dhkim]

Please let met know if you need a reproduction of the problem!

[0x80]

@0916dhkim What does your monorepo folder structure look like? Note that isolate-package only supports monorepos with a flat packages directory structure. There can be multiple packages container directories, but each needs to be flat.

I use isolate-package in projects that have internal dependencies depending on other internal dependencies, so it was designed to handle that, but both packages need to be living in the same parent folder next to each other and not nested.

This is also described in prerequisites.

[0916dhkim]

@0x80 Yes, my company's project is a flat monorepo 👍
The packages are not nested (as in no package is placed inside another), but some internal packages depend on other internal packages. So the dependency tree is deep.

I'll put together a small repro repo to demonstrate the issue soon because I cannot share the private code.

[0916dhkim]

Here is the repro!
https://github.com/0916dhkim/_isolate-error-repro

Let me know if I can provide more details about the error.

[0x80]

@0916dhkim thanks I'll try to look into it soon. I'm currently travelling and don't have much time behind my computer.

[0x80]

I found the issue. You need to specify in each package manifest, what files are supposed to be published. This is also described in the readme prerequisites.

The paths in the generated pnpm lockfile are correct. They should be relative to the root of the isolate directory, and not to each other. to ./packages/first and ./packages/second are both correct.

This is that I added:

"files": [
    "main.js"
  ],

"files": [
    "first.js"
  ],

 "files": [
    "second.js"
  ],

Also isolate-package can be a direct dev dependency on the package that you are isolating, so no need to place it in the root.

Let me know how you get on...

I will make a note to add this files field check and throw and error otherwise.

[0916dhkim]

@0x80 Thank you! I will try to add files manifest and see if that fixes the issue.

[0916dhkim]

I tried adding files, but it still results in ERR_MODULE_NOT_FOUND. (Updated the repro repository)

When I look at other open source repos, it seems like pnpm dependencies are linked relative to each importer. Tho I am not 100% sure what is the officially supported behavior.

Example:
@prisma/accelerate-contract depends on @prisma/client, and pnpm-lock.json links to link:../client instead of link:./packages/client.
https://github.com/prisma/prisma/blob/ea868e9aa54e71fd1832c846cca74ec8d0280020/pnpm-lock.yaml#L204-L208

I'm currently travelling and don't have much time behind my computer.

No rush for a response! I really like the library & the ergonomics of the firebase-tools-with-isolate too 🙌

[0x80]

Strange. I don't see the problem yet. If you look at mono-ts, the services/fns package depends on common and backend, but backend also depends on common. I can isolate that package, and do a pnpm i and execute node dist/index.js without problems.

Maybe you can spot the difference... I'm out of time for now.

[0916dhkim]

@0x80 I identified the difference!

In mono-ts, @repo/fns uses tsup bundler to build. Because all internal modules are resolved & bundled together during the build step before isolate, the isolated output doesn't need to resolve path to @repo/common or @repo/backend.

That is why it runs fine even though the generated pnpm-lock.yaml points to incorrect paths. When you inspect /services/fns/isolate/dist/index.js file, you can see it does not import any @repo/* package.

[0x80]

@0916dhkim Sounds plausible. It would be a major flaw and I'm surprised that you are the first to run into this issue. I will try your fix in the coming days, and added a question to your proposed code.

[0916dhkim]

Thank you 🙏

[0x80]

@0916dhkim Would you like to wrap this up? I'd like to merge your PR before anyone else runs into the problem. I posted a code suggestion, but didn't do an official review + request changes.