-
Notifications
You must be signed in to change notification settings - Fork 405
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
WIP - fix(logger-plugin): log async completion in separate group #1269
base: master
Are you sure you want to change the base?
Conversation
private getActionLogHeader() { | ||
const actionName = getActionTypeFromInstance(this.action); | ||
const formattedTime = formatTime(this.startedTime); | ||
const message = `action ${actionName} (started @ ${formattedTime})`; |
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.
is redunant, you can
return `action ${actionName} (started @ ${formattedTime})`;
In addition, I recommend always describing the return type, since in the future you can forget to return an object instead of a returned string.
private getActionLogHeader(): string {
}
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.
Agreed. Will do
} | ||
|
||
errored(error: any) { | ||
if (this.synchronousWorkEnded) { |
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.
What is synchronousWorkEnded
?
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.
ActionLogger
instance is created when any action is dispatched. As actions can be sync and async this is a flag that basically says "oh I've completed doing my synchronous job".
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.
I think for some reason this is all very complicated, but oh well
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.
Need more comments I guess
await promise; | ||
|
||
// Assert | ||
const expectedCallStack = [ |
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.
I suggest also adding a test that will call three asynchronous actions and see how it will look in the logs
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.
Great idea. Especially if they call each other.
Will we see this in the release? |
No, probably not. This will require quite a bit more code to resolve and I am spending all my available time on reviewing PRs 😄 |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: #276, #213 and #561
Currently the logger writes an action to the console with one group that closes after the action completes. This works great if all actions only do synchronous work but creates a mess in the console as soon as there is asynchronous completion of actions.
What is the new behavior?
If an action will complete asynchronously then the console group for the action is closed after all synchronous work is done and a new group is opened when the asynchronous completion or error happens.
Does this PR introduce a breaking change?
Other information