Skip to content

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

Merged
zkochan merged 7 commits intomainfrom
gluxon/read-workspace-packages-cliOptions-in-config
Jun 17, 2024
Merged

fix: use --workspace-packages CLI over pnpm-workspace.yaml in more cases#8215
zkochan merged 7 commits intomainfrom
gluxon/read-workspace-packages-cliOptions-in-config

Conversation

@gluxon
Copy link
Copy Markdown
Member

@gluxon gluxon commented Jun 16, 2024

Changes

Implementing feedback from: #8213 (comment)

Let's read cliOptions['workspace-packages'] when computing config.workspacePackagePatterns, rather than doing it later in pnpm/src/main.ts.

pnpm/src/main.ts Outdated
engineStrict: config.engineStrict,
nodeVersion: config.nodeVersion ?? config.useNodeVersion,
patterns: cliOptions['workspace-packages'] ?? config.workspacePackagePatterns,
patterns: config.workspacePackagePatterns ?? ['.', '**'],
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is something I noticed in one of the other PRs too. The patterns field here could accidentally be undefined, but TypeScript never caught this because cliOptions is Record<string, any>.

I think this only comes up in tests where workspaceDir is directly passed rather than through the findWorkspaceDir function. In production, the findWorkspaceDir verifies pnpm-workspace.yaml exists, so config.workspacePackagePatterns is defined.

I added ?? ['.', '**'] to match the existing behavior, but I think we should default to ?? [] in this case so broken tests fail.

@gluxon gluxon marked this pull request as ready for review June 16, 2024 20:13
@gluxon gluxon requested a review from zkochan as a code owner June 16, 2024 20:13
@gluxon gluxon changed the title fix: use --workspace-packages CLI over pnpm-workspace.yaml in more cases fix: use --workspace-packages CLI over pnpm-workspace.yaml in more cases Jun 16, 2024
Base automatically changed from gluxon/hoist-manifest-read-to-config to main June 16, 2024 23:26
@zkochan zkochan requested a review from fireairforce as a code owner June 16, 2024 23:26
@zkochan zkochan merged commit 7f4cea4 into main Jun 17, 2024
@zkochan zkochan deleted the gluxon/read-workspace-packages-cliOptions-in-config branch June 17, 2024 00:49
engineStrict?: boolean
nodeVersion?: string
patterns: string[]
patterns?: string[]
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zkochan I was a bit surprised by this change. Should we fix the tests and make this required again?

Changing this to be optional makes it harder to merge in #8214.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants