refactor: improve type checking when finding workspace packages by gluxon · Pull Request #8214 · pnpm/pnpm

gluxon · 2024-06-16T06:14:32Z

PR Stack

Problem

After #8213, code within pnpm is expected to use the patterns argument when calling findWorkspacePackages. This avoids the pnpm-workspace.yaml file from being read multiple times, which could cause strange consistency issues if the file is modified while pnpm is running. It also just feels silly to re-read this file when it's already in-memory.

However, because the patterns argument is optional, it's easy to forget to pass in a value for this argument.

Changes

Making patterns required when calling findWorkspacePackages.

findWorkspacePackages should be used within the pnpm repo.
findWorkspacePackagesUsingManifest is now a convenience function that code outside of pnpm can call. This function continues to read pnpm-workspace.yaml automatically.

gluxon · 2024-06-16T06:21:21Z

completion/plugin-commands-completion/src/complete.ts

-      const allProjects = await findWorkspacePackages(workspaceDir, {
+      const workspaceManifest = await readWorkspaceManifest(workspaceDir)
+      const allProjects = await findWorkspacePackagesUsingPatterns(workspaceDir, {
+        patterns: workspaceManifest?.packages ?? ['.', '**'],


I added ?? ['.', '**'] here since that's what plugin-commands-completion is currently doing, but this feels like a bug to me. If there's no pnpm-workspace.yaml, I would think allProjects should be empty?

You may still use pnpm i -r even outside a workspace.

gluxon · 2024-06-16T06:25:41Z

pkg-manager/plugin-commands-installation/test/add.ts

    linkWorkspacePackages: false,
    saveWorkspaceProtocol: 'rolling',
    workspaceDir: process.cwd(),
+    workspaceManifest: { dir: process.cwd(), contents: workspaceManifest },


It's a bit annoying that tests calling the install/add/import handlers need to pass in workspaceManifest directly.

One way to fix this would be to better distinguish between a workspaceManifest field that's (a) not yet read or (b) read, but empty. We would then call readWorkspaceManifest in the former case.

But that's a lot of complexity to make a few tests easier to write. I decided not to make the workspaceManifest config field more complicated.

gluxon · 2024-06-16T06:32:13Z

pnpm/src/main.ts

      engineStrict: config.engineStrict,
      nodeVersion: config.nodeVersion ?? config.useNodeVersion,
-      patterns: cliOptions['workspace-packages'] ?? config.workspaceManifest?.contents.packages,
+      patterns: cliOptions['workspace-packages'] ?? config.workspaceManifest?.contents.packages ?? ['.', '**'],


This uncovered a potential problem with pnpm install --recursive. This is a case where a missing pnpm-workspace.yaml actually means everything is part of the workspace. I think this is a bug?

The current behavior of findWorkspacePackages() is:

Use opts.patterns if provided.

Use pnpm-workspace.yaml's packages field if the file is present.

Use ['.', '**'] if pnpm-workspace.yaml is not present.

Although the patterns argument here on line 203 is non-null, undefined could get passed in here since noUncheckedIndexedAccess is false.

Is it intended that pnpm install --recursive treats all folders with package.json files as workspace packages even when pnpm-workspace.yaml isn't on disk? That's the current behavior here. I added ?? ['.', '**'] to make this more clear and to satisfy the required patterns argument.

gluxon · 2024-06-16T06:43:15Z

Opening this for review, but it's not required for catalogs. I wanted to see what this would look like if we made improvements to the type safety of patterns. Curious for thoughts.

zkochan · 2024-06-16T12:30:15Z

Why don't we just make it a required option and remove the possibility of reading the workspace manifest from within findWorkspacePackages?

gluxon · 2024-06-16T18:22:42Z

That was roughly my intention with this PR.

There were 3 approaches I was considering:

Require patterns in findWorkspacePackages.
Require patterns in findWorkspacePackages, but leave around a findWorkspacePackagesUsingManifest function that reads pnpm-workspace.yaml for you. pnpm wouldn't use this, but other people using @pnpm/workspace.find-packages might find it convenient.
Add a new function (i.e. findWorkspacePackagesUsingPatterns) that requires patterns, leaving findWorkspacePackages as-is.

I took the 3rd approach, but happy to change to either (1) or (2).

They all roughly have the effect of requiring patterns for code within pnpm, but have different requirements for people externally consuming the @pnpm/workspace.find-packages NPM package. I chose (3) since it was the one that didn't require a new major version of @pnpm/workspace.find-packages.

zkochan · 2024-06-16T23:38:02Z

This needs a rebase.

gluxon · 2024-06-17T00:58:09Z

Sounds good. I'll rebase now.

Did you have a preference on whether we should introduce a new function for convenience of @pnpm/workspace.find-packages users?

gluxon · 2024-06-17T05:54:33Z

Latest force push contains a few notable changes:

A few if (opts.workspaceDir) conditions needed to be updated to if (opts.workspaceDir && opts.workspacePackagePatterns).
I decided to switch the names. The findWorkspacePackages now requires patterns and there's a new findWorkspacePackageUsingManifest for external NPM package users.
I had to revert the change that made patterns optional for filterPackagesFromDir. fix: use --workspace-packages CLI over pnpm-workspace.yaml in more cases #8215 (comment)

gluxon · 2024-06-17T05:56:58Z

pnpm/src/main.ts

      engineStrict: config.engineStrict,
      nodeVersion: config.nodeVersion ?? config.useNodeVersion,
-      patterns: config.workspacePackagePatterns,
+      patterns: config.workspacePackagePatterns ?? ['.', '**'],


Here's my prior comments on this here:

fix: use --workspace-packages CLI over pnpm-workspace.yaml in more cases #8215 (comment)

fix: use --workspace-packages CLI over pnpm-workspace.yaml in more cases #8215 (comment)

I think the ?? ['.', '**'] is ugly and should be [] instead, but this would be a behavior change and breaks a few tests. I'm looking into fixing these tests, but wanted to push up a rebase for this PR in the meantime.

I have made patterns optional, not default value is needed.

One thing I was worried about with patterns being optional is developers in the future writing code in pnpm forgetting to pass in a value when calling findWorkspacePackages. Everything might work as expected when testing, but in production packages might get included from the default filter ['.', '**'] that shouldn't, and then users will get confused.

I think that's a general footgun with optional parameters.

zkochan · 2024-06-17T09:33:33Z

pkg-manager/plugin-commands-installation/src/link.ts

+  if (opts.workspaceDir && opts.workspacePackagePatterns) {
    workspacePackagesArr = await findWorkspacePackages(opts.workspaceDir, {
      ...opts,
      patterns: opts.workspacePackagePatterns,


Maybe just add a default value for patterns in findWorkspacePackages. Otherwise the tests have to be updated with workspacePackagePatterns, which isn't even used in most cases because allProjects is passed.

I noticed that above too. #8214 (comment)

A lot of tests have a common options setup that defaults values from @pnpm/config. Perhaps we could set those to ['.', '**'] for tests.

gluxon changed the title ~~Gluxon/workspace find packages patterns~~ refactor: improve type checking when finding workspace packages Jun 16, 2024

gluxon mentioned this pull request Jun 16, 2024

refactor: store workspace manifest in @pnpm/config for reuse #8213

Merged

gluxon commented Jun 16, 2024

View reviewed changes

gluxon marked this pull request as ready for review June 16, 2024 06:33

gluxon requested review from fireairforce and zkochan as code owners June 16, 2024 06:33

gluxon force-pushed the gluxon/hoist-manifest-read-to-config branch 2 times, most recently from 9136a66 to a26819b Compare June 16, 2024 19:24

Base automatically changed from gluxon/hoist-manifest-read-to-config to main June 16, 2024 23:26

gluxon mentioned this pull request Jun 17, 2024

fix: use --workspace-packages CLI over pnpm-workspace.yaml in more cases #8215

Merged

gluxon added 10 commits June 17, 2024 01:17

refactor: factor out a FindWorkspacePackagesOpts interface

e18a833

break: require workspacePackagePatterns arg to findWorkspacePackages

131682d

feat: add findWorkspacePackagesUsingManifest for external convenience

b4dd8b5

refactor: update findWorkspacePackages in plugin-commands-installation

eb10be9

refactor: update findWorkspacePackages in plugin-commands-completion

433b03c

refactor: update findWorkspacePackages in filter-workspace-packages

e26c992

fix: name pnpm-workspace.yaml correctly in fixtures

8d072c9

test: pass workspaceManifest to installation commands in tests

252e62c

test: pass patterns to findWorkspacePackages in tests

2773444

refactor: explicitly match all packages in filterPackagesFromDir call

3858dd6

gluxon force-pushed the gluxon/workspace-find-packages-patterns branch from 0560060 to 3858dd6 Compare June 17, 2024 05:51

gluxon commented Jun 17, 2024

View reviewed changes

test: fix

b004b78

zkochan reviewed Jun 17, 2024

View reviewed changes

zkochan added 6 commits June 17, 2024 11:46

refactor: findWorkspacePackages

a2b453c

refactor: workspace find packages patterns

503d618

fix: compile

6e4952c

refactor: create @pnpm/workspace.filter-packages-from-dir

ac0d964

refactor: create @pnpm/workspace.filter-packages-from-dir

e0e8102

test: fix

f8489f7

zkochan requested a review from weyert as a code owner June 17, 2024 13:05

zkochan approved these changes Jun 17, 2024

View reviewed changes

zkochan added 2 commits June 17, 2024 16:32

test: fix

efac346

test: fix

b68de23

zkochan merged commit 75a98e1 into main Jun 17, 2024

zkochan deleted the gluxon/workspace-find-packages-patterns branch June 17, 2024 15:34

gluxon mentioned this pull request Jun 26, 2024

Catalogs Tracking Issue #7072

Open

18 tasks

Uh oh!

Conversation

gluxon commented Jun 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Stack

Problem

Changes

Uh oh!

gluxon Jun 16, 2024

Choose a reason for hiding this comment

Uh oh!

zkochan Jun 17, 2024

Choose a reason for hiding this comment

Uh oh!

gluxon Jun 16, 2024

Choose a reason for hiding this comment

Uh oh!

gluxon Jun 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gluxon commented Jun 16, 2024

Uh oh!

zkochan commented Jun 16, 2024

Uh oh!

gluxon commented Jun 16, 2024

Uh oh!

zkochan commented Jun 16, 2024

Uh oh!

gluxon commented Jun 17, 2024

Uh oh!

gluxon commented Jun 17, 2024

Uh oh!

gluxon Jun 17, 2024

Choose a reason for hiding this comment

Uh oh!

zkochan Jun 17, 2024

Choose a reason for hiding this comment

Uh oh!

gluxon Jun 17, 2024

Choose a reason for hiding this comment

Uh oh!

zkochan Jun 17, 2024

Choose a reason for hiding this comment

Uh oh!

gluxon Jun 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gluxon commented Jun 16, 2024 •

edited

Loading

gluxon Jun 16, 2024 •

edited

Loading

gluxon Jun 17, 2024 •

edited

Loading