Feat: Add env support to remove hardcoding the prod urls #1144
Conversation
seriouslysean
added
enhancement
| @@ -4,28 +4,28 @@ | |||
| <meta charset="utf-8" /> | |||
| <meta http-equiv="X-UA-Compatible" content="IE=edge" /> | |||
| <meta name="viewport" content="width=device-width,initial-scale=1.0" /> | |||
| <link rel="icon" href="/favicon.ico" /> | |||
| <link rel="icon" href="%VITE_APP_URL%/favicon.ico" /> | |||
There was a problem hiding this comment.
@Haviles04 I'm looking at you, shame! You hardcoded all these productions urls, hahaha. @itsalaidbacklife we should get rid of these and switch to this env method. @jerryh-jr will need it in his unhead PR anyway.
There was a problem hiding this comment.
Actually, Mal did it here, 7826a83, but no one fixed it hahahaha.
There was a problem hiding this comment.
I take it this is using ejs variable substitution, right? Where is this exact syntax coming from, and why does it not require reference to either process.env or meta.import to access the env var?
There was a problem hiding this comment.
C'mon, bro, read the docs https://vite.dev/guide/env-and-mode.html#html-env-replacement 😏
There was a problem hiding this comment.
I mean touché but given that we have 4 different tools for rendering dynamic html between Vue, vite, sails and ejs, it's not always clear which docs to read when seeing a new templating syntax 🤷
There was a problem hiding this comment.
puts hands up it wasn't me!
| @@ -1,3 +1,4 @@ | |||
| VITE_APP_URL=http://localhost:8080 | |||
There was a problem hiding this comment.
I probably missed some, someone really double check me. @Haviles04 @itsalaidbacklife
There was a problem hiding this comment.
Additional files using localhost:
vite.config.mjsconfig/env/staging.js
Places I'm unsure whether we want to change the hard coded references:
- tests - I'd think we want to keep the tests commands e.g.
rankedMatches.spec.jsto use localhost incy.request()to avoid accidentally testing against prod and generating bs data src/plugins/sails.js- fallback in case of missing env; seems reasonable to keep it
There was a problem hiding this comment.
We should not change HOST_SERVER_URL in vite.config as it's used for the docker version, and server only. Same for config/env/staging.js, they should stay hardcoded since it's based on hard url location. I also think tests are fine staying hardcoded for that same reason.
Issue number
Relevant issue number
Please check the following
Please describe additional details for testing this change