fix: getToggle by name may not find the toggle

[gastonfournier]

About the changes

Node SDK interface should notify that getting a toggle by name can return undefined if the toggle does not exist:
https://github.com/Unleash/unleash/blob/d298b04cb2864b4853ca456e402714c98adaf932/src/lib/proxy/proxy-repository.ts#L75-L77

[coveralls]

Coverage: 91.617%. Remained the same when pulling cfdfd6c on find-may-not-find into 55274e4 on main.

[thomasheartman]

Looks like a sensible change to me: I love that we're explicit about returning undefined if the feature doesn't exist 😄 But I've got a question on relaxing function parameters: I'm not sure we should allow isFeatureEnabled to be called with an undefined feature. 🤔 But that said, I'm guessing we already do that today, so this PR only updates the code to reflect reality and does not do anything to address any issues?

[thomasheartman]

Why are we changing the accepted parameter value? Surely, there's no point in ever calling this with a feature that's undefined? Wouldn't it be better to only allow you to call it with a defined feature?

[gastonfournier]

yes! Very good point, this is just the result of type refactoring, but I can look into this as it definitely makes sense!

[sighphyre]

I think the more interesting question is: why is this method public? It's called by isEnabled which takes a string, resolves a toggle and calls this with the results and therefore and be called with an undefined. But what else is actually using this?

[gastonfournier]

Ok, I analyzed the option of always receiving something defined, but that'd be a breaking change. Right now this method accepts undefined but the type system does not reflect that. The proof is this line in the method body:

unleash-client-node/src/client.ts

Lines 73 to 76 in 55274e4

	isFeatureEnabled(feature: FeatureInterface, context: Context, fallback: Function): boolean {
	if (!feature) {
	return fallback();
	}

So, we're not changing the parameter value, we're just making it reflect the reality

[sighphyre]

Spec tests pass, change looks reasonable LGTM

[ivarconr]

Nice. I consider this a bug-fix.