feat: New data kinds for edge SDKs. #260
Conversation
| @@ -44,7 +44,7 @@ | |||
| "eslint-plugin-prettier": "^5.0.0", | |||
| "prettier": "^3.0.0", | |||
| "typedoc": "0.23.26", | |||
| "typescript": "^5.1.6" | |||
There was a problem hiding this comment.
The package.json changes will come from main once I merge main in, but I want things in a good state before I attempt to do that.
| default: | ||
| throw new Error(`Unsupported DataKind: ${namespace}`); | ||
| callback(null); |
There was a problem hiding this comment.
If we don't know what it is, then we should just return null I think. (Or empty objects in the all cases.) Versus throwing an exception and then returning null.
If I had not noticed this, then it would have ran, but the edge SDKs would likely have slowed down quite a bit attempting to access fields that didn't exist.
| @@ -59,8 +59,14 @@ export class EdgeFeatureStore implements LDFeatureStore { | |||
| case 'segments': | |||
| callback(item.segments[dataKey]); | |||
| break; | |||
| case 'configurationOverrides': | |||
There was a problem hiding this comment.
These two new types are used for event sampling, it will be nice to support that in edge SDKs as well.
kinyoklion
requested review from
yusinto and
louis-launchdarkly
as code owners
| callback(item.configurationOverrides?.[dataKey] ?? null); | ||
| break; | ||
| case 'metrics': | ||
| callback(item.metrics?.[dataKey] ?? null); |
There was a problem hiding this comment.
Is there a difference between passing null and undefined? Usually there's no difference:
| callback(item.configurationOverrides?.[dataKey] ?? null); | |
| break; | |
| case 'metrics': | |
| callback(item.metrics?.[dataKey] ?? null); | |
| callback(item.configurationOverrides?.[dataKey]); | |
| break; | |
| case 'metrics': | |
| callback(item.metrics?.[dataKey]); |
There was a problem hiding this comment.
So, functionally not a lot in this specific case. Here though the LDFeatureStore is still compatible with the one we had before, and the typing had LDFeatureStoreItem | null. So it just has 'null' here to keep typescript compiling. Without it it simply would not compile.
At some point we may change that typing. I have to release 9.0 for migrations, so it would possibly be a good time.
Not in this situation, but in regards to truthiness, it can be very important. Thankfully ?. and ?? have reduced it.
If 0/"" is valid, but null/undefined are not, then you often would end up with checks that check for both.
Add new data kind support as well as a couple error handling changes.