Skip to content

Cleanup/Reorganize app.js#42

Open
nick-w-nick wants to merge 11 commits into
Canvasbird:masterfrom
nick-w-nick:express-updates
Open

Cleanup/Reorganize app.js#42
nick-w-nick wants to merge 11 commits into
Canvasbird:masterfrom
nick-w-nick:express-updates

Conversation

@nick-w-nick

Copy link
Copy Markdown

Hello, in this PR I've gone ahead and cleaned up/reorganized the app.js file.

The changes include:

  • replacing body-parser with the built-in express middleware
  • fixed misc typos
  • updated variable declarations to ES6
  • reorganized all middleware and route imports to an "on-demand" pattern (import module only when it actually needs to be used)
  • added the helmet middleware which grants automatic generic security policies
  • modified server startup messaging to include crash log

I'm planning to apply this structure to the rest of the codebase in future commits but thought I'd start with the root of the application initially.

Let me know if you need any changes/etc.

Thanks!

@ronaldtorres ronaldtorres left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think that "reorganized middleware imports and usage" is not a useful change as doesn't follow clean code best practices. All dependencies must be declared to the top of the file. This allow other developers to see quickly what dependencies are used in the current file.

@nick-w-nick

nick-w-nick commented Oct 14, 2021

Copy link
Copy Markdown
Author

@ronaldtorres I understand, it's just a specific practice I've adhered to for my app.js files without any issues.

Regardless, I think the modification within the commit I just made brings it back to a cleaner state while still retaining the usage organization.

Let me know if this change works for you.

@ronaldtorres

Copy link
Copy Markdown

@nick-w-nick yes! that change is more clean for me...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants