-
Notifications
You must be signed in to change notification settings - Fork 135
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.
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
error #649
Conversation
OPEN_NEXT_ERROR_LOG_LEVEL is the minimal error level from which internal errors are logged. It can be set to - "0" / "debug" - "1" / "warn" (default) - "2" / "error"
🦋 Changeset detectedLatest commit: 1f9cbe8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
if (error.logLevel === 0) { | ||
// Display the name and the message instead of full Open Next errors. | ||
return debug( | ||
...args.map((arg) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@conico974 the code does not display the full exception here and for level 1. Let me know what you think about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's good, as long as we have the message it's fine
Coverage Report
File Coverage
|
commit: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One little thing, but otherwise LGTM
if (error.logLevel === 0) { | ||
// Display the name and the message instead of full Open Next errors. | ||
return debug( | ||
...args.map((arg) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's good, as long as we have the message it's fine
return; | ||
} | ||
if (error.logLevel === 0) { | ||
// Display the name and the message instead of full Open Next errors. | ||
return debug( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably add some kind of warning just before this line in case openNextDebug
is not set, otherwise there will be no error logged and it might be confusing for people.
Or we just use console.log
or console.error
and rely entirely on OPEN_NEXT_ERROR_LOG_LEVEL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea.
+1 to drop openNextDebug
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM Thanks
Thanks for the review! |
@@ -1,4 +1,4 @@ | |||
import type { BaseOpenNextError } from "utils/error"; | |||
import { type BaseOpenNextError, isOpenNextError } from "utils/error"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that BaseOpenNextError
is not used anywhere in this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch -> #653
No description provided.