[ENHANCEMENT] update the typing for getFeatureVariable to reflect what the docs say will be returned
Pomax opened this issue · 7 comments
Description
The documentation for getFeatureVariable over on https://docs.developers.optimizely.com/full-stack-experimentation/docs/get-feature-variable-javascript-node#section-generic says that this function will return a boolean, integer, double, or string depending on what it sees stored, but the return type is marked as unknown.
This should be boolean | number | string | null, instead. Or, even better, an OptimizelyVariableType with an exported type definition export type OptimizelyVariableType = boolean | number | string | null; that other code can import and work with as needed.
Benefits
Since the return type for getFeatureVariable isn't unknown, but is very much known, making the typing reflect that would be super useful for folks who are writing their own mocks/clients with typing based on what the documentation claims may be returned.
Risks/Downsides
unknown is incompatible with everything except another unknown, so by updating the return typing to only those types that might actually be returned by the function, there shouldn't be any downsides. Only upsides.
Hello @Pomax , thanks for the suggestion! I agree that we should return specific type instead of unknown. We will look to get this done soon.
Side Question to @trishahanlon: Are the /full-stack-experimentation/docs meant to be published in addition to the newer /feature-experimentation/docs?
Linking to PR #887
@mikechu-optimizely, yes, /full-stack-experimentation/docs and /feature-experimentation/docs are still both valid.
/full-stack-experimentation/docscontains docs for theIsFeatureEnabled,Activate,GetFeatureVariable, andTracklegacy methods./feature-experimentation/docscontians the docs for the "new"DecideandTrack Eventmethods.
Thanks @trishahanlon
@Pomax , #887 was merged and will be available in the next 5.x(-beta) release.
awesome! thank you
This has been released in v5.0.0.