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

chore: suppress "ConditionError" in Logger #6213

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

chore: suppress "ConditionError" in Logger #6213

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
723cebe
suppress RTK-Query condition abort warning
sviderock Nov 5, 2024
79ce1f8
add test
sviderock Nov 5, 2024
a67cf54
move condition from Logger to sagas
sviderock Nov 5, 2024
9007d4f
remove console.log
sviderock Nov 5, 2024
1b17fe6
remove new line
sviderock Nov 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions src/utils/Logger.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,23 @@
if (this.level < LoggerLevel.Debug) {
return
}

/**
* This is not an actual error but rather a way for Redux Toolkit to signal that a thunk
* condition (most likely caused by RTK-Query) has evaluated to false and cancelled the thunk
* execution OR the thunk execution was aborted. As can be seen from the Redux Toolkit code,
* this is not an actual Error class instance but just a way of signaling the failed condition
* necessary to execute the thunk.
*
* For more context, please check out the following links:
* https://github.com/reduxjs/redux-toolkit/blob/7af5345eaeab83ca57b439aec41819420c503b34/packages/toolkit/src/createAsyncThunk.ts#L596-L602
* https://stackoverflow.com/questions/69789058/what-does-this-error-mean-in-redux-toolkit
* https://redux-toolkit.js.org/api/createAsyncThunk#options
*/
if (messages?.[0]?.error?.name === 'ConditionError') {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just in case, I've ran a check in node_modules to ensure no other dependency is throwing ConditionError. The only library using is is Redux Toolkit and the only place where this particular error is thrown is in the only place in their codebase that I've linked there in the comment (first link).

So it is safe to assume that suppressing this error will not suppress any errors that we do not want to suppress.

Copy link
Member

Choose a reason for hiding this comment

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

What do you think of doing this in the logger saga instead?

wallet/src/redux/sagas.ts

Lines 77 to 101 in 34a9d38

function* loggerSaga() {
const devModeActive: boolean = yield* select(devModeSelector)
if (!devModeActive) {
return
}
yield* takeEvery('*', (action: UnknownAction) => {
if (
action?.type &&
(action.type.includes('IDENTITY/') || loggerPayloadBlocklist.includes(action.type))
) {
// Log only action type, but not the payload as it can have sensitive
// information or information that is not helpful for debugging. Excluding
// all IDENTITY/ actions because high likelyhood they contain PII and the
// blocklist may get out of date.
Logger.debug('redux/saga@logger', `${action.type} (payload not logged)`)
return
}
try {
Logger.debug('redux/saga@logger', action)
} catch (err) {
Logger.warn('redux/saga@logger', 'could not log action of type', action.type)
}
})
}

This way it's more targeted. And the logger doesn't need to know about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jeanregisser I think it makes sense, will do!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jeanregisser updated

return

Check warning on line 55 in src/utils/Logger.ts

View check run for this annotation

Codecov / codecov/patch

src/utils/Logger.ts#L55 

Added line #L55 was not covered by tests
}

console.debug(tag, ...messages)
}

Expand Down
Loading