Fixed broken local devserver due to env vars

[xela1601]

Description

While i was looking for the reason, my local devserver is not working anymore, I realized there was a typo 🙈 and several env vars were undefined which causes the next-server to crash

Within process.env.ZETKIN_GEN2_ORGANZE_URL an I was missing.
This caused the following error when running yarn devserver and going to localhost:3000:

Also as the error logs say env var FEAT_AREAS is missing (also INSTANCE_OWNER_HREF, INSTANCE_OWNER_NAME and MUIX_LICENSE_KEY).
I resolved this by adding them as empty env vars to .env.development

Screenshots

Changes

• fixed typo within Add documentation about environment variables #2452 : process.env.ZETKIN_GEN2_ORGANZE_URL -> process.env.ZETKIN_GEN2_ORGANIZE_URL
• added INSTANCE_OWNER_HREF, INSTANCE_OWNER_NAME and MUIX_LICENSE_KEY as empty env vars to .env.development

Notes to reviewer

• not sure if these empty env vars are meant to be set to empty values for the dev server and if it makes sense to also set them this way within .env.test

[richardolsson]

Hey, nice work catching this and tracing it back to mistakes that me and @henrycatalinismith made in #2452.

The misspelling of "ORGANIZE" is obviously just a typo that needs to be fixed, as you've done here.

But the problem with the unserializable variables, might not be best solved by requiring them, but by handling their undefined state. This was done before #2452 by falling back to null, but that logic was removed in 3614805 (see layout.tsx).

I'm not sure whether I prefer just falling back to null or requiring them. If we really do require them, I reckon we should throw an error when they're missing.

What do you think @xela1601 and @henrycatalinismith?

[henrycatalinismith]

Ah cool! I had wondered what the point was with the type being string | undefined | null instead of just string | undefined.

My understanding of what I've read here in this thread is that what's problematic is when the envVars prop returned by the getServerSideProps function generated by scaffold contains undefined values. So the purpose that the | null served in this type was to enable blank environment variables to be passed through here without triggering this error.

My next question after understanding this is: why is it so necessary to include these empty values at all? Do we have functionality in some of the page components that depends on being able to enumerate all possible environment variables, whether blank or otherwise? If not, I think it seems a little strange to add a whole other kind of blank value to this union type just to allow ourselves to do something that serves no practical purpose. If that's the case, perhaps we could try something like this instead so that we exclude undefined environment variables from the return value of getServerSideProps instead of smuggling them through it in a trenchcoat.

diff --git a/src/utils/next.ts b/src/utils/next.ts
index 7a57da808..9ef37aa39 100644
--- a/src/utils/next.ts
+++ b/src/utils/next.ts
@@ -205,7 +205,8 @@ export const scaffold =
     if (hasProps(result)) {
       result.props = {
         ...result.props,
-        envVars: {
+        envVars: Object.fromEntries(
+          Object.entries({
             FEAT_AREAS: process.env.FEAT_AREAS,
             INSTANCE_OWNER_HREF: process.env.INSTANCE_OWNER_HREF,
             INSTANCE_OWNER_NAME: process.env.INSTANCE_OWNER_NAME,
@@ -213,7 +214,8 @@ export const scaffold =
             ZETKIN_APP_DOMAIN: process.env.ZETKIN_APP_DOMAIN,
             ZETKIN_GEN2_ORGANIZE_URL: process.env.ZETKIN_GEN2_ORGANZE_URL,
             ZETKIN_PRIVACY_POLICY_LINK: process.env.ZETKIN_PRIVACY_POLICY_LINK,
-        },
+          }).filter(([_, value]) => value !== undefined)
+        ),
         lang,
         messages,
         user: ctx.user,

If this can possibly work I think it might be preferable on the basis that the intention and purpose are much more immediately clear from the code. It's clear in the above that we're filtering out undefined values and even without a helpful comment the reader can begin to guess at why that might be happening. By contrast, sneaking them through here as nulls and adding || null everywhere we read the variables is a bit more cryptic-looking and much harder to attach an explanatory comment to given how widespread the || nulls become.

[richardolsson]

I don't think it's that weird to say that these config variables need to be either null or have a valid value. But I can see why undefined | null comes off as weird, and that undefined needs to be there for other reasons (mocking for testing comes to mind).

I don't find your solution @henrycatalinismith very readable (and I suspect it will cause typing errors because Object.fromEntries() returns a too generic type), but that can be easily resolved by moving that logic into a nicely named utility function which will take care both of the typing (via the return type) and the explaining (via the name).

[henrycatalinismith]

Been daydreaming about this all day, it's a fun little puzzle. Here's a whole other option.

diff --git a/src/utils/next.ts b/src/utils/next.ts
index 7a57da808..d2259daa9 100644
--- a/src/utils/next.ts
+++ b/src/utils/next.ts
@@ -1,4 +1,5 @@
 import { getIronSession } from 'iron-session';
+import { isUndefined, omitBy } from 'lodash';
 import { ParsedUrlQuery } from 'querystring';
 import {
   GetServerSideProps,
@@ -205,7 +206,8 @@ export const scaffold =
     if (hasProps(result)) {
       result.props = {
         ...result.props,
-        envVars: {
+        envVars: omitBy(
+          {
             FEAT_AREAS: process.env.FEAT_AREAS,
             INSTANCE_OWNER_HREF: process.env.INSTANCE_OWNER_HREF,
             INSTANCE_OWNER_NAME: process.env.INSTANCE_OWNER_NAME,
@@ -214,6 +216,8 @@ export const scaffold =
             ZETKIN_GEN2_ORGANIZE_URL: process.env.ZETKIN_GEN2_ORGANZE_URL,
             ZETKIN_PRIVACY_POLICY_LINK: process.env.ZETKIN_PRIVACY_POLICY_LINK,
           },
+          isUndefined // Undefined values are not JSON serializable
+        ),
         lang,

Possible to further refactor a utility function out of this like so.

const omitUndefined = (obj) => _.omitBy(obj, _.isUndefined);

[xela1601]

I personally like the idea of the omitUndefined function. This makes it very clean and understandable to me. I also like the idea making use of lodash-utility functions in general. So in the end it would look something like this, right?

envVars: omitUndefined({
   FEAT_AREAS: process.env.FEAT_AREAS,
   INSTANCE_OWNER_HREF: process.env.INSTANCE_OWNER_HREF,
   INSTANCE_OWNER_NAME: process.env.INSTANCE_OWNER_NAME,
   ZETKIN_GEN2_ORGANIZE_URL: process.env.ZETKIN_GEN2_ORGANIZE_URL,
   ZETKIN_PRIVACY_POLICY_LINK: process.env.ZETKIN_PRIVACY_POLICY_LINK,
})

[richardolsson]

I personally like the idea of the omitUndefined function. This makes it very clean and understandable to me. I also like the idea making use of lodash-utility functions in general. So in the end it would look something like this, right?

envVars: omitUndefined({
   FEAT_AREAS: process.env.FEAT_AREAS,
   INSTANCE_OWNER_HREF: process.env.INSTANCE_OWNER_HREF,
   INSTANCE_OWNER_NAME: process.env.INSTANCE_OWNER_NAME,
   ZETKIN_GEN2_ORGANIZE_URL: process.env.ZETKIN_GEN2_ORGANIZE_URL,
   ZETKIN_PRIVACY_POLICY_LINK: process.env.ZETKIN_PRIVACY_POLICY_LINK,
})

Yeah, I like this solution! 💯

[xela1601]

Shall I implement it this way?
Where is the best location for that helper function, or should I put it into the same file?

[richardolsson]

Shall I implement it this way? Where is the best location for that helper function, or should I put it into the same file?

Please do! You can put it in src/utils.

[xela1601]

done :)

[richardolsson]

Nice work! I think this is where it needs to be now. I added a comment below just because I want to hear your take on it.

Will merge when I've heard back!

[richardolsson]

Based on the answer from Alex's LLM (🤔) on my comment in the last review, I assumed I was right about the bundle size. I tried changing the imports via Github, which of course failed. So I made a change locally and force-pushed it.

With that, I think this is ready to merge!