Skip to content

fix: report correct errors when using the pnpm server#7032

Merged
zkochan merged 1 commit intopnpm:mainfrom
gluxon:fix-error-when-installing-with-store
Sep 2, 2023
Merged

fix: report correct errors when using the pnpm server#7032
zkochan merged 1 commit intopnpm:mainfrom
gluxon:fix-error-when-installing-with-store

Conversation

@gluxon
Copy link
Copy Markdown
Member

@gluxon gluxon commented Sep 2, 2023 •
edited
Loading

Steps to Reproduce

  1. Start the pnpm store server.

    ❯ pnpm server start

  2. Run a command that is expected to error.

    ❯ pnpm install is-positive@1.0.1
Cannot read properties of undefined (reading 'code')

    In this example, we're attempting to install a version that doesn't exist.

Changes

The actual error is now printed with the fix in this PR.

❯ pnpm install is-positive@1.0.1
A store server is running. All store manipulations are delegated to it.
 ERROR  No matching version found for is-positive@1.0.1

This error happened while installing a direct dependency of /Users/gluxon/Developer/test

Explanation

Errors from the pnpm server go through a different reporting code path. It looks like in this case the err object is empty for the log argument.

pnpm/cli/default-reporter/src/reporterForServer.ts

Line 24 in ee61ca4

console.log(reportError(log, config))

@gluxon gluxon force-pushed the fix-error-when-installing-with-store branch from 9485b9b to 72a963a Compare September 2, 2023 04:47
gluxon
gluxon commented Sep 2, 2023
View reviewed changes
Comment thread cli/default-reporter/src/reportError.ts
export function reportError (logObj: Log, config?: Config) {
const errorInfo = getErrorInfo(logObj, config)
let output = formatErrorSummary(errorInfo.title, logObj['err']['code'])
let output = formatErrorSummary(errorInfo.title, (logObj as LogObjWithPossibleError).err?.code)
Copy link
Copy Markdown
Member Author

@gluxon gluxon Sep 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I would normally add a test when fixing a bug, but in this case the bug happened as a result of bypassing TypeScript compilation checks through the [] indexing syntax. The new LogObjWithPossibleError interface here should make this lookup a bit safer in the future.

@gluxon gluxon changed the title fix: fix error reporting when using the pnpm server fix: report correct errors when using the pnpm server Sep 2, 2023
@gluxon
Copy link
Copy Markdown
Member Author

gluxon commented Sep 2, 2023

Here's one case of a user reporting this. #4889

In that issue the cause of the error was fixed, but I think there's still a bug when reporting the error.

@gluxon gluxon marked this pull request as ready for review September 2, 2023 05:52
@gluxon gluxon requested a review from zkochan as a code owner September 2, 2023 05:52
zkochan
zkochan reviewed Sep 2, 2023
View reviewed changes
Comment thread .changeset/wet-lies-smoke.md Outdated
@gluxon gluxon force-pushed the fix-error-when-installing-with-store branch from 72a963a to 4c43163 Compare September 2, 2023 18:42
@zkochan zkochan merged commit cc785f7 into pnpm:main Sep 2, 2023
@gluxon gluxon deleted the fix-error-when-installing-with-store branch September 2, 2023 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zkochan zkochan zkochan approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@gluxon @zkochan