Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 1 commit into from Sep 2, 2023
Merged

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

merged 1 commit into from Sep 2, 2023

Conversation

gluxon
Copy link
Member

@gluxon gluxon commented Sep 2, 2023 •
edited

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
cli/default-reporter/src/reportError.ts
@@ -21,7 +21,7 @@ const colorPath = chalk.gray

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
Member Author

@gluxon gluxon Sep 2, 2023 •
edited

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
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
@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
6 of 8 checks passed
@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.

None yet
2 participants
@gluxon @zkochan