dynamically load GoCardless handlers #557
Conversation
actual-github-bot
bot
changed the title
matt-fidd
changed the title
WalkthroughThe pull request introduces a significant refactoring of the bank module loading mechanism in Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
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
🧹 Nitpick comments (1)
src/app-gocardless/bank-factory.js (1)
10-24: Consider switching to an asynchronous directory read and adding error handling.Currently,
fs.readdirSyncblocks the event loop. Usingfs.promises.readdirkeeps the code fully async. Additionally, consider handling unexpected files or modules without default exports to prevent runtime errors.If you’d like, you can replace these lines as shown:
- const bankHandlers = fs - .readdirSync(banksDir) - .filter((filename) => filename.includes('_') && filename.endsWith('.js')); + const bankHandlers = (await fs.promises.readdir(banksDir)) + .filter((filename) => filename.includes('_') && filename.endsWith('.js'));You could also wrap the
import()calls in atry/catchblock to handle broken or missing default exports gracefully.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/557.mdis excluded by!**/*.md
📒 Files selected for processing (1)
src/app-gocardless/bank-factory.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Docker image (alpine)
🔇 Additional comments (3)
src/app-gocardless/bank-factory.js (3)
1-3: Neat usage of Node.js ESM imports.These imports are consistent with ESM best practices. No issues or improvements are needed here.
7-9: Good approach to replicate__dirnamein ESM.Using
fileURLToPath(import.meta.url)withpath.resolveis a clean, standard way to manage relative file paths in ESM.
26-26: Verify Node.js environmental support for top-level await.Node 16+ with
"type": "module"is required for top-level await. Run the following script to verify if your project’s Node version is set to 16 or higher:✅ Verification successful
Environment fully supports top-level await ✓
The project is correctly configured with Node.js >=18.0.0 and "type": "module", meeting all requirements for top-level await usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Checking package.json for Node engine requirements and .nvmrc if present fd 'package.json' . | xargs cat | jq '.engines' fd '.nvmrc' . | xargs catLength of output: 96
Script:
#!/bin/bash # Checking package.json for type:module configuration fd 'package.json' . | xargs cat | jq '.type'Length of output: 51
Hmm.. perhaps. I don't know too much about docker. If I understand correctly we might be able to define a specific directory that it would look in for "extras" and mount that as a volume |
|
I'll merge this after the freeze since it doesn't actually do anything different to master |
No description provided.