remove axios dependency #503
Conversation
actual-github-bot
bot
changed the title
WalkthroughThe pull request introduces changes to the error handling logic within the Possibly related PRs
Suggested reviewers
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/app-gocardless/app-gocardless.js (2)
Line range hint
239-249: Implementation still depends on AxiosThe changes appear inconsistent with the PR objective of removing the Axios dependency. While the code has been modified to check the
isAxiosErrorproperty directly, it still:
- Relies on Axios-specific error properties (
error.isAxiosError)- Uses Axios response patterns (
error.response?.data)Consider implementing a custom error handler that doesn't depend on Axios patterns. Here's a suggested approach:
-case error.isAxiosError: +case error.name === 'NetworkError': console.log( 'Something went wrong', - inspect(error.response?.data || error, { depth: null }), + inspect(error.details || error, { depth: null }), ); sendErrorResponse({ error_type: 'SYNC_ERROR', - error_code: 'NORDIGEN_ERROR', + error_code: 'NETWORK_ERROR', }); break;
Line range hint
239-262: Consider refactoring error handling for better maintainabilityThe current error handling structure has some duplication and inconsistencies in logging. Consider extracting the error mapping logic to a separate function for better maintainability.
Here's a suggested refactoring:
const ERROR_MAPPINGS = { NetworkError: { error_type: 'SYNC_ERROR', error_code: 'NETWORK_ERROR' }, GenericGoCardlessError: { error_type: 'SYNC_ERROR', error_code: 'NORDIGEN_ERROR' }, // ... other mappings }; function logError(error, context = '') { console.log( `Something went wrong${context ? ` - ${context}` : ''}`, inspect(error, { depth: null }) ); } function handleTransactionError(error) { const errorType = error.constructor.name; const errorMapping = ERROR_MAPPINGS[errorType] || { error_type: 'UNKNOWN', error_code: 'UNKNOWN', reason: 'Something went wrong' }; logError(error, errorType); return { ...errorMapping, details: error.details }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/503.mdis excluded by!**/*.md
📒 Files selected for processing (1)
src/app-gocardless/app-gocardless.js(1 hunks)
🔇 Additional comments (1)
src/app-gocardless/app-gocardless.js (1)
Line range hint 239-249: Verify the impact of removing Axios
Since the PR aims to remove the Axios dependency, we should verify all potential uses of Axios in the codebase to ensure complete removal.
#!/bin/bash
# Search for any remaining Axios-related code
echo "Searching for Axios imports..."
rg -i "axios" --type js
echo "Searching for Axios error handling patterns..."
rg "isAxiosError|error\.response\?\.data" --type js
echo "Checking package.json for axios dependency..."
if [ -f "package.json" ]; then
jq '.dependencies.axios, .devDependencies.axios' package.json
fi
|
Closing, root cause of the error is not what we thought |
Reported in discord, this is causing an issue because it's not in the package.json
To avoid adding a large dependency I've rolled our own here, referencing https://github.com/axios/axios/blob/5d54d22321b3c3d29c5dff5e8086cd9db7e2d101/lib/helpers/isAxiosError.js#L12