-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Commenting app.js for clarity #1053
base: master
Are you sure you want to change the base?
Conversation
Added some comments for clarity of middlewares
backend/src/app.js
Outdated
@@ -2,43 +2,43 @@ const express = require('express'); | |||
|
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.
maybe it's better if you add this comment here:
// Import required packages
@@ -2,43 +2,43 @@ const express = require('express'); | |||
|
|||
const cors = require('cors'); | |||
const compression = require('compression'); | |||
|
|||
const cookieParser = require('cookie-parser'); | |||
|
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.
and here:
// Import routers and controllers
backend/src/app.js
Outdated
// create our Express app | ||
const app = express(); | ||
|
||
// enable CORS with specific options | ||
app.use( | ||
cors({ | ||
origin: true, |
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.
here you can add:
// Allow requests from any origin
backend/src/app.js
Outdated
// create our Express app | ||
const app = express(); | ||
|
||
// enable CORS with specific options | ||
app.use( | ||
cors({ | ||
origin: true, | ||
credentials: true, |
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.
and here:
// Allow sending cookies along with requests
app.use(cookieParser()); | ||
app.use(express.json()); | ||
app.use(express.urlencoded({ extended: true })); | ||
|
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.
here you can add:
// Compress HTTP responses
updated app.js according to review
Added some comments for clarity of middlewares
Description
-- Changed adminAuth to coreAdminAuth to keep up with the file trend.
-- added some comments for basic clarity
Note - This is my first pull request so I'm sorry if it silly, i do hope it gets merged. Thank you for letting me contribute(even if its just commenting).
Steps to Test
no need
Checklist