-
Notifications
You must be signed in to change notification settings - Fork 2
feat: using node best practices and some improvements #1
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
Conversation
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.
Pull Request Overview
This PR modernizes the codebase by moving to ESM, updating to Fastify v5, adding robust schema validation, integrating lint/format tooling, and improving tests and deployment scripts
- Migrate CommonJS to EcmaScript modules across app, routes, plugins, and tests
- Introduce JSON schema validation for new endpoints and expand
/accountstest coverage - Add ESLint/Prettier configuration, update dependencies, and include a Heroku deploy button
Reviewed Changes
Copilot reviewed 17 out of 21 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Update metadata, bump dependencies, add lint/format scripts |
| src/app.js | Migrate to ESM, configure Swagger & Autoload plugins |
| src/routes/index.js | Convert to ESM, add schema validation for /unitofwork and Data Cloud routes |
| test/routes/accounts.test.js | Convert tests to ESM and expand /accounts test cases |
| eslint.config.js | Add ESLint and Prettier integration |
| README.md | Enhance documentation, add Heroku Deploy button & usage instructions |
Comments suppressed due to low confidence (3)
README.md:59
- The README refers to
/api-docs, but by default the Swagger UI is exposed at/documentation. Please update the path to match the actual route.
- **GET /api-docs** - Interactive Swagger UI for API documentation
README.md:60
- The docs list
/healthbut the code registers/healthcheck. Align the documentation with the code or adjust the route name for consistency.
- **GET /health** - Health check endpoint
src/routes/index.js:77
- New routes
/unitofworkand/handleDataCloudDataChangeEventhave been added, but there are no corresponding tests. Consider adding tests to cover their validation logic and response behavior.
fastify.post(
package.json
Outdated
| "name": "applink-getting-started-nodejs", | ||
| "version": "1.0.0", | ||
| "description": "Heroku AppLink sample app with Fastify", | ||
| "description": "Heroku AppLink Sammple App with Fastify", |
Copilot
AI
Jul 15, 2025
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.
There is a typo in the description: "Sammple" should be "Sample".
| "description": "Heroku AppLink Sammple App with Fastify", | |
| "description": "Heroku AppLink Sample App with Fastify", |
src/routes/index.js
Outdated
| throw new Error(errorMessage); | ||
| } | ||
|
|
||
| return reply; |
Copilot
AI
Jul 15, 2025
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.
[nitpick] Returning the reply object without sending any payload can be confusing. You might remove this return or explicitly call reply.send() to clarify the intended behavior.
| return reply; | |
| reply.send({ success: true }); |
Major changes: