chore: add new parsing function for env var numbers

[thomasheartman]

The new function builds on the old one, but adds a min option, so that you can set a lower bound for the result. The min value will also affect the default value and the options override if they are lower than the min value.

Discussion points:

Should it be its own function instead? Maybe. I did that in the first implementation. It's valid, although I'd say it might be more unwieldy when used. Additionally having two functions for the same type might be confusing.

What do we want the API to be? I added an optionsOverride. Why not just use a single default and let the caller provide it correctly? As was pointed out in #7938, it's easy to put optionsOverride || fallback. But because 0 is falsy, it might lead to unexpected results. So by encapsulating that in the function, we avoid that.

Non-goals

I have not added a max option because we don't have a use case for it right now and because it would introduce additional complexity: what if max is lower than min?

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Skipped Deployments

Name	Status	Preview	Comments	Updated (UTC)
unleash-docs	⬜️ Ignored (Inspect)	Visit Preview		Aug 21, 2024 7:42am
unleash-monorepo-frontend	⬜️ Ignored (Inspect)	Visit Preview		Aug 21, 2024 7:42am

[github-actions]

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

OpenSSF Scorecard

Package	Version	Score	Details

Scanned Manifest Files

[gastonfournier]

I was actually thinking of having another function, but having the min here might be ok. When I see options it looks like the function is doing too much.

Maybe instead of having:

parseEnvVarNumber(
            process.env.UNLEASH_FEATURE_ENVIRONMENT_STRATEGIES_LIMIT,
            30,
            {
                min: 1,
                optionsOverride:
                    options?.resourceLimits?.featureEnvironmentStrategies,
            },
        )

you can do something like:

validateMin(1, parseEnvVarNumber(
            process.env.UNLEASH_FEATURE_ENVIRONMENT_STRATEGIES_LIMIT,
            options?.resourceLimits?.featureEnvironmentStrategies ?? 30
        ))

My idea was that this should fail if you provide the wrong value, leaning on fail fast rather than allowing you to have issues at runtime.

But if we just want to ignore the user input (in this case the negative value), I'd stick with your solution of using Math.max function from before.

I still think that in this case of a startup configuration parameter, it's best to fail fast than to assume the customer input is invalid and then add a minimum value of our own, because we might be doing something the customer does not expect

[thomasheartman]

When I see options it looks like the function is doing too much.

Just for clarity: are you talking about the whole options object, or the optionsOverride property?

I still think that in this case of a startup configuration parameter, it's best to fail fast than to assume the customer input is invalid and then add a minimum value of our own

Yeah, and I still don't agree with this. I don't think we do this with any other values, right? Sure, if we can't connect to the database, we die, but not because we verify it.

Also, what would be the use case for setting negative numbers? Or in this case, 0 for strategies? Without strategies, Unleash doesn't work (or you know, flags don't do anything). (But then again, we all know users do the wildest things, so 💁🏼 )

I'd stick with your solution of using Math.max function from before.

Yeah, I don't mind that, to be honest. The one thing I do think that's nice here is the fact that it does the ?? for us and that it's tested. But it's pretty small, and not that important anyway 🤷🏼

[thomasheartman]

In this case, we could also just define another function in the same file and use that when defining the values:

    const resourceLimit = ({
        envVarName,
        min,
        fallback,
        option,
    }) =>
        Math.max(
            min,
            parseEnvVarNumber(process.env[envVarName], option ?? fallback),
        );

        featureEnvironmentStrategies: resourceLimit({
            envVarName: 'UNLEASH_FEATURE_ENVIRONMENT_STRATEGIES_LIMIT',
            min: 1,
            fallback: 30,
            option: options?.resourceLimits?.featureEnvironmentStrategies,
        })

But it doesn't really address any of the other bits here and it'd be untested.

[gastonfournier]

Just for clarity: are you talking about the whole options object, or the optionsOverride property?

About the whole options object that includes the optionsOverride. Usually, having an options parameter is a code-smell because means that the function is less composable and it fulfills more than one purpose (which can be modified or disabled by the use of different options).

Also, what would be the use case for setting negative numbers? Or in this case, 0 for strategies? Without strategies, Unleash doesn't work (or you know, flags don't do anything). (But then again, we all know users do the wildest things, so 💁🏼 )

Indeed, the whole point of failing is to tell them not to add a negative number. Probably their intention is different than just having the value 1 (otherwise they would just configure 1, right?). So, I rather tell them, "hey! this is invalid, you can't do this" instead of: "Yeah, I see you want to set a negative number, but I will assume you wanted to say 1 because I don't understand why you want to set this to negative" (now that I think about it, maybe they want infinite... and one would be way lower than infinite)

[gastonfournier]

Sorry @thomasheartman I was expecting someone from DX to look into it, but it didn't happen, I don't have strong feelings about this, just different ways of doing the same, but it's correct so I'm ✔️ so you're not blocked waiting on an approval

[thomasheartman]

no worries; I didn't ask anyone else to look into this, so I wasn't really expecting it to go anywhere. On the whole, I don't need this to be merged. If you don't think it's a good idea, then we can just close the PR instead. This is something we can come back to in the future if we need to.

[gastonfournier]

If we don't need it I'd say we just close it. We currently have two approaches, we'd have a third one if we ask someone else, which means we don't have very convincing reasons to go one way or the other (also don't have strong opinions against), so maybe we just let it go for now, until we have a valid reason to do this.