workos / authkit-nextjs

The WorkOS library for Next.js provides convenient helpers for authentication and session management using WorkOS & AuthKit with Next.js.
MIT License
68 stars 19 forks source link

env vars loaded on require / import #45

Closed robcresswell closed 2 months ago

robcresswell commented 6 months ago

https://github.com/workos/authkit-nextjs/blob/3eaea54ac89648a70077128b3e9c9dc96c7e2e5c/src/env-variables.ts

I was curious if there was any particular reason for the loading of env vars on require, rather than via some kind of load function. I'm not super familiar with next, but this seems fairly painful because it means you need your environment to be correct just to build the code, let alone run it?

PaulAsjes commented 6 months ago

Hey @robcresswell, can you elaborate on why it's painful for you? The library needs all those values to function, so we thought it would be better to fail fast if any are missing.

robcresswell commented 6 months ago

Hey @PaulAsjes, thanks for the quick response.

Its mostly around testing. Things like build steps and simple smoke tests ("is the server up and replying to healthchecks") can't be carried out without needing to run in an environment that has all of our WorkOS configuration. There are workarounds for this like dynamic imports AFAIK. It may also be an "us" problem; as I said, not super familiar with next so it might just be my bad usage.

PaulAsjes commented 6 months ago

Thanks for elaborating! The library only checks if the env variables are set, not if they are correct or not. Meaning you can put any string value in there and it'll work, with the exception of WORKOS_COOKIE_PASSWORD which does need to be 32 characters long.

I think your situation might be a bit of an edge case, which shouldn't be too hard to circumvent. When starting your dev server for testing you can pass in the variables via the command line to make things easier for you when testing, e.g.

WORKOS_API_KEY=foo WORKOS_CLIENT_ID=bar WORKOS_REDIRECT_URI=baz WORKOS_COOKIE_PASSWORD=longstringthatisatleast32characterslong next dev

Does that help?

robcresswell commented 6 months ago

Sure; we're aware there are workarounds, I just thought it was worth flagging that this might cause issues. If its not really seen as a problem, I think we can close this issue 😄

robcresswell commented 5 months ago

If anyone else stumbles across this, we've ended up with the following in one of our containers:

RUN env \
    WORKOS_CLIENT_ID=must-be-set-to-lint \
    WORKOS_API_KEY=must-be-set-to-lint \
    WORKOS_REDIRECT_URI=data:must-be-set-to-lint \
    WORKOS_COOKIE_PASSWORD=must-be-set-to-lint-and-must-be-really-long \
    npm run build

Still not a big fan of this behaviour, but it is what it is cc @PaulAsjes

Niklex21 commented 4 months ago

Another edge case is when deploying this on a cloud with environment variables only exposed during runtime, rather than build (for security purposes). In that same scenario, the build would fail even though it is correct.

Does anyone know of a good workaround for this besides setting dummy values?

robcresswell commented 4 months ago

@Niklex21 We're still just setting dummy values wherever we use it, unfortunately

PaulAsjes commented 4 months ago

Re-opening this as it's clearly still a problem. We initially implemented thinking it would be a DX improvement, but clearly it's having the opposite effect for those developing in cloud environments.

Will have a rethink on how to handle this.

willredington commented 2 months ago

Yeah just to add to this, we deploy all our nextjs stuff in kubernetes, so being able to build a docker image without exposing mock work OS variables would be pretty nice