-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
fix(structured-logging): fix wrongly reporting status as success when we should still be in pending state #26380
fix(structured-logging): fix wrongly reporting status as success when we should still be in pending state #26380
Conversation
Gatsby Cloud Build Reportclient-only-paths 🎉 Your build was successful! See the Deploy preview here. Build Details🕐 Build time: 18s PerformanceLighthouse report
|
Gatsby Cloud Build Reportusing-styled-components 🎉 Your build was successful! See the Deploy preview here. Build Details🕐 Build time: 18s PerformanceLighthouse report
|
Gatsby Cloud Build Reportgatsby-master 🎉 Your build was successful! See the Deploy preview here. Build Details🕐 Build time: 1m PerformanceLighthouse report
|
c0e53d2
to
0a6c431
Compare
Gatsby Cloud Build Reportusing-reach-skip-nav 🎉 Your build was successful! See the Deploy preview here. Build Details🕐 Build time: 22s PerformanceLighthouse report
|
Gatsby Cloud Build Reportgatsby 🎉 Your build was successful! See the Deploy preview here. Build Details🕐 Build time: 22m PerformanceLighthouse report
|
0a6c431
to
ecedcb6
Compare
Gatsby Cloud Build Reportgatsby 🎉 Your build was successful! See the Deploy preview here. Build Details🕐 Build time: 23m |
…ET_STATUS dispatch
const actionsToEmit: ActionsToEmit = [] | ||
|
||
const logsState = getStore().getState().logs | ||
|
||
const globalStatus = getGlobalStatus(id, status) | ||
|
||
if (globalStatus !== logsState.status) { | ||
actionsToEmit.push(setStatus(globalStatus)) | ||
} | ||
|
||
actionsToEmit.push({ | ||
type: Actions.PendingActivity, | ||
payload: { | ||
id, | ||
type: ActivityTypes.Pending, | ||
status, | ||
return [ | ||
setStatus(globalStatus), | ||
{ | ||
type: Actions.PendingActivity, | ||
payload: { | ||
id, | ||
type: ActivityTypes.Pending, | ||
status, | ||
}, | ||
}, | ||
}) | ||
|
||
return actionsToEmit | ||
] |
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.
gist of this and other activity related code changes is that we always would use setStatus
action creator as it internally already checks for global status (or pending global status for delayed dispatchs)
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 hope I won't have to debug these test cases failing at some point. I love that you wrote them but predict they will be hard to figure out how to fix if they fail.
Anyways, lgtm :)
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 like tests! 🙏
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.
This is good. Definitely an improvement (other than the fix)
Successfully published:
|
Description
The
setStatus
action creator calls are not working correctly right now. This PR aims to add bunch of tests first (some of which will fail), and add code changes showing that hose fixes some problems later.