This makes it possible to just check whether opts.workspaceManifest is set and have both opts.workspaceManifest.dir and opts.workspaceManifest.contents be typed as non-null.

if (opts.workspaceManifest) {
  await findWorkspacePackages(opts.workspaceManifest.dir, {
    ...opts,
    patterns: opts.workspaceManifest.contents.packages
  })
}

@zkochan Adding a workspacePackagePatterns field to the top-level of Config makes it harder to write the code above.

We have to check that both workspacePackagePatterns and workspaceDir are non-null, but that's a bit of a code smell because they're supposed to either be both defined, or both undefined.

if (opts.workspaceDir != null && opts.workspacePackagePatterns != null) {
  await findWorkspacePackages(opts.workspaceDir, {
    ...opts,
    patterns: opts.workspacePackagePatterns
  })
}

This is a problem for the next PR in #8214.

Maybe we can do something like this in the Config object?

interface Config {
  workspace?: {
    dir: string;
    packagePatterns: string;

    // other existing fields might also make sense here
    hoistWorkspacePackages?: boolean;
  }
}

Similar to the comment above, the optional chain here isn't necessary if we check opts.workspaceManifest is non-null instead of opts.workspaceDir.

That's a potentially larger refactor though. Tackling this in a different PR.

I think we just need to add the setting from workspace manifest to the rest of the settings. Why should the users of the config be aware where the settings are coming from? Just add workspacePatterns to the Config object.

That's a good point. I was attempting to follow the rootProjectManifest option below, which grouped settings under where it comes from. It sounds like that wasn't a good example to follow?

What I do like about the existing setup here is that it better models what can happen at runtime. Grouping options together like this makes it possible to declare many options that are all non-null/defined if any of them are non-null. This means we don't have to handle non-existent states at compile time. In this case, we know both the workspace dir and patterns are non-null if either are non-null.

In a future PR, we'd also know the patterns and dir if catalogs are defined. And that intuitively makes sense since catalogs are a feature that only work in a workspace.

Add a new setting to the config object that will prioritize the workspace-packages cli flag over the. packages from workspace manifest. Here you'll just need to do

I can add the new config.workspacePackagePatterns field, but do you mind if I fold the cliOptions['workspace-packages'] in a separate PR?

I just want to do a bit more testing to ensure there's no behavior changes. Today, cliOptions['workspace-packages'] is only read here in this if (cliOptions['recursive']) { block. I want to make sure things don't unexpectedly break if that CLI option takes priority always elsewhere we use patterns.

It is fine if it will take priority always.

Can I do this in another PR after this merges? I wanted this PR to just be a refactor without behavior changes. The change above might subtly change how pnpm behaves.

I understand we want cliOptions['workspace-packages'] to take precedence always, but the problem is it doesn't today in all cases. Fixing that might introduce other bugs.

It doesn't matter. That flag was added for one specific user (who seems like moved to bun) and we don't even have the flag documented on the website.

You can do a separate PR but nobody will notice the difference.

Gotcha. I was probably a bit too afraid of risk in this case. I'm glad to hear nobody will notice either way.

Making this change in #8215.