Skip to content

Commit

Permalink
Add/fix concurrent dev environment and more quality of life improveme…
Browse files Browse the repository at this point in the history
…nts (#8)
  • Loading branch information
oroth8 authored Apr 4, 2023
1 parent f0bcfc0 commit c9ecdaa
Show file tree
Hide file tree
Showing 14 changed files with 230 additions and 442 deletions.
1 change: 1 addition & 0 deletions be/.gitignore
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
# Keep environment variables out of version control
.env
/src/logs/**/*
15 changes: 9 additions & 6 deletions be/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import cookieParser from 'cookie-parser'
import { ErrorMiddleware } from './src/middlewares/error.middleware.js'
import swaggerJSDoc from 'swagger-jsdoc'
import swaggerUi from 'swagger-ui-express'
import url from 'url'

loadEnv()

Expand All @@ -26,9 +27,9 @@ export class App {
this.proxied_api_url = PROXIED_API_URL || ''
this.proxied_api_token = PROXIED_API_TOKEN || ''

this.initializeRoutes(routes)

This comment has been minimized.

Copy link
@vanderhoop

vanderhoop Apr 4, 2023

Collaborator

By moving this up, do we lose out on the middleware?

This comment has been minimized.

Copy link
@oroth8

oroth8 Apr 4, 2023

Author Collaborator

@vanderhoop correct, good catch. Order should be:

this.initializeMiddlewares()
this.initializeRoutes(routes)
this.initializeFrontend()
this.initializeErrorHandling()
this.initializeSwagger()

tracking here: #11

this.initializeFrontend()
this.initializeMiddlewares()
this.initializeRoutes(routes)
this.initializeSwagger()
this.initializeErrorHandling()
}
Expand All @@ -48,14 +49,16 @@ export class App {

initializeFrontend() {
// Use build folder for static files
this.app.use(express.static('build'))
if (this.env === 'production') {

This comment has been minimized.

Copy link
@vanderhoop

vanderhoop Apr 4, 2023

Collaborator

This is the one bit of important architectural feedback. We'll want to avoid the slippery slope of "environment sniffing" in the source code, as over time, folks will see this pattern and inevitably replicate the conditional checks (and even add to them). Ideally, the source code has no idea what environment it's in, and instead just does as it's told/configured.

More concretely, you want the ENV VARs you use to configure the application declaratively.

In this case we could set an env var called SERVE_STATIC_BUILD_DIR (or similar), and set it to true only in dev, and then use that as the conditional.

This comment has been minimized.

Copy link
@vanderhoop

vanderhoop Apr 4, 2023

Collaborator

As a cautionary tale, at Peloton, they had many 'production-esque' environments that weren't actually production, and they ended up with all of these crazy checks like if (NODE_ENV === "production" && !window.location.origin.includes("--preview")) { X } else { Y }

And the logical bloat ended up causing bugs that cost them cold hard cash.

This comment has been minimized.

Copy link
@oroth8

oroth8 Apr 4, 2023

Author Collaborator

@vanderhoop yes completely agree with you. The current setup was a symptom of some of the logic introduced in the backend template that I just copied over. I would love to refactor this. I created an issue here (more of todos list). https://github.com/LaunchPadLab/veritext-replacement-template/issues?q=is%3Aissue+is%3Aopen+sort%3Aupdated-desc

this.app.use(express.static('build'))
}
this.app.get('/env', exposeEnvMiddleware(loadPublicEnv))

// TODO ADD ERROR HANDLING FOR UNKNOWN ROUTES
// const __dirname = url.fileURLToPath(new URL('.', import.meta.url))
// this.app.get('*', (req, res) =>
// res.sendFile(path.join(__dirname, '../', '/build/index.html'))
// )
const __dirname = url.fileURLToPath(new URL('.', import.meta.url))
this.app.get('*', (req, res) =>
res.sendFile(path.join(__dirname, '../', '/build/index.html'))
)
}

initializeProxy() {
Expand Down

This file was deleted.

44 changes: 0 additions & 44 deletions be/src/logs/debug/2023-03-31.log

This file was deleted.

This file was deleted.

20 changes: 0 additions & 20 deletions be/src/logs/error/2023-03-31.log

This file was deleted.

Loading

0 comments on commit c9ecdaa

Please sign in to comment.