Remove AppConstants #5044
Remove AppConstants #5044
Conversation
|
Preview URL 🚀 : https://blurts-server-pr-5044-mgjlpikfea-uk.a.run.app |
| @@ -26,6 +25,15 @@ import { SerializedSubscriber } from "../../../next-auth"; | |||
| import { record } from "../../functions/server/glean"; | |||
| import { renderEmail } from "../../../emails/renderEmail"; | |||
| import { SignupReportEmail } from "../../../emails/templates/signupReport/SignupReportEmail"; | |||
| import { getEnvVarsOrThrow } from "../../../envVars"; | |||
|
|
|||
| const envVars = getEnvVarsOrThrow([ | |||
There was a problem hiding this comment.
Oh that's pretty cool, nice idea. I didn't like how non-standard AppConstants was but I really like the behavior of throwing upfront.
| @@ -108,6 +107,6 @@ export async function POST(req: NextRequest) { | |||
| } | |||
| } else { | |||
| // Not Signed in, redirect to home | |||
| return NextResponse.redirect(AppConstants.SERVER_URL, 301); | |||
| return NextResponse.redirect(process.env.SERVER_URL ?? "/", 301); | |||
There was a problem hiding this comment.
I'm curious why you don't use the getEnvVarsOrThrow function here? Seems like you could avoid having to do the ?? "/", is this fallback ever desirable?
There was a problem hiding this comment.
That's a good question, and I was (still am) on the fence. My reasoning was as follows: AppConstants only logged an error without throwing, so if this variable wouldn't be set, this would just result in a log and a call to NextResponse.redirect(undefined, 301). So if I were to use that function instead, maybe I would be causing API routes that used to succeed but return a malformed response, to start failing upfront now. Not in practice, because the variable should be set, I think - but I might be thinking wrong.
It looks like .redirect supports relative URLs, so judging by the comment above this line, this fallback is the desired behaviour. Though given that this is a POST request handler, I don't see why that makes sense - so presumably, we're never hitting this.
| @@ -50,7 +49,7 @@ export async function POST(req: NextRequest) { | |||
| existingEmail.email, | |||
| ); | |||
| return NextResponse.redirect( | |||
| AppConstants.SERVER_URL + "/user/settings", | |||
| process.env.SERVER_URL + "/user/settings", | |||
There was a problem hiding this comment.
Same question re: getEnvVarsOrThrow vs. process.env
There was a problem hiding this comment.
Same reasoning as #5044 (comment) - in this case, if it's unset, it would fall back to redirecting to /user/settings anyway, I think.
| !FX_REMOTE_SETTINGS_WRITER_USER || | ||
| !FX_REMOTE_SETTINGS_WRITER_PASS || | ||
| !FX_REMOTE_SETTINGS_WRITER_SERVER | ||
| ) { |
There was a problem hiding this comment.
Seems like you could use getEnvVarsOrThrow and catch it + process.exit (I think throwing from here would the process to exit anyway but being explicit is probably better just in case it's refactored later)
There was a problem hiding this comment.
Yeah, I was actually considering that too, but then I figured I'd have to wrap the entire file in a try {} block, which... Isn't that big of a deal, but felt a little bit icky, so I stuck with mostly matching the existing code. I hadn't realised yet that throwing would cause an exit anyway, so that could actually be a good compromise too. What do you think?
|
@rhelmer Heh, my answers to all your questions are basically: good point, here's my reasoning, but I could go either way, what do you think? So, do you have opinions? :P |
To make the migration as safe as possible, in places where it was used to access environment variables that were absolutely required, I replaced it with a function that forces the caller to explicitly enumerate the environment variables they specifically expect, allows them to access them in a type-safe way, and throws if the named environment variables aren't set. (This is slightly different than the previous behaviour, where AppConstants would only log the missing variable without throwing, but I checked that all these variables should be set in production.)
Vinnl
force-pushed
the
remove-appconstants
branch
from
13a5bd2
to
e2f656a
Compare
|
Cleanup completed - database 'blurts-server-pr-5044' destroyed, cloud run service 'blurts-server-pr-5044' destroyed |
To make the migration as safe as possible, in places where it was used to access environment variables that were absolutely required, I replaced it with a function that forces the caller to explicitly enumerate the environment variables they specifically expect, allows them to access them in a type-safe way, and throws if the named environment variables aren't set. (This is slightly different than the previous behaviour, where AppConstants would only log the missing variable without throwing, but I checked that all these variables should be set in production.)
Merges into #5043.