feat: Convert custom_attribute_condition_evaluator to TS #561
Conversation
4856aca to
408e825
Compare
| type Condition = { | ||
| [name: string]: string; | ||
| }; |
There was a problem hiding this comment.
Let's be more specific here. You can check out the structure of condition objects in the datafile by creating audiences with different condition types.
Condition objects have type, name, match, and value properties. The functions in this module use name, match, and value.
|
|
||
| /** | ||
| * Returns true if the value is valid for exact conditions. Valid values include | ||
| * strings, booleans, and numbers that aren't NaN, -Infinity, or Infinity. | ||
| * @param value | ||
| * @returns {Boolean} | ||
| * @returns {?boolean} |
There was a problem hiding this comment.
Should this just be boolean, not ?boolean?
| import { isNumber, isSafeInteger } from '../../utils/fns'; | ||
| import { | ||
| LOG_LEVEL, | ||
| LOG_MESSAGES, | ||
| } from '../../utils/enums'; | ||
|
|
||
| var MODULE_NAME = 'CUSTOM_ATTRIBUTE_CONDITION_EVALUATOR'; | ||
| // TODO: Find place for UserAttributes and Condition types to live in |
There was a problem hiding this comment.
We could create a shared types module at lib/shared_types.ts, and put this there.
It would be nice if somehow we could import < 10000 code class="notranslate">UserAttributes from there and use it here, as well as also in lib/index.d.ts. I had some trouble figuring that out, it may not be possible. If so I think we can just duplicate the definition (have one in lib/index.d.ts, and one in lib/shared_types.ts which is imported by out TS modules).
There was a problem hiding this comment.
I also tried to use UserAttributes in lib/index.d.ts from lib/shared_types.ts i, but could not figure it out how to make it work, so left lib/index.d.ts as is and followed your suggestion.
9bbbd51 to
408e825
Compare
packages/optimizely-sdk/lib/core/custom_attribute_condition_evaluator/index.ts
Outdated
Show resolved
Hide resolved
| * TODO: Change to accept and object with named properties | ||
| */ | ||
| export var evaluate = function(condition, userAttributes, logger) { | ||
| var conditionMatch = condition.match; | ||
| export function evaluate(condition: Condition, userAttributes: UserAttributes, logger: LoggerFacade): boolean | null { |
There was a problem hiding this comment.
This is a good opportunity to clean up logging in this module.
- Let's change all these functions so that they use a logger defined inside this module (like
const logger = getLogger(MODULE_NAME);, and then remove theloggerargument from the list of parameters. - Call the logger method corresponding to the desired level, rather than passing in level (
logger.debug(...)instead oflogger.log(LOG_LEVEL.DEBUG, ...)
| if (!conditionMatch) { | ||
| evaluatorForMatch = exactEvaluator; | ||
| } else { | ||
| evaluatorForMatch = EVALUATORS_BY_MATCH_TYPE[conditionMatch]; | ||
| } | ||
| if (!evaluatorForMatch) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Assuming the whole codebase was TS and passed type checking, I don't think control can ever reach line 88. But the compiler will complain if we remove this if (!evaluatorForMatch) check. We don't want to return null without logging a warning message like in the beginning of this function.
Let's do this instead:
| if (!conditionMatch) { | |
| evaluatorForMatch = exactEvaluator; | |
| } else { | |
| evaluatorForMatch = EVALUATORS_BY_MATCH_TYPE[conditionMatch]; | |
| } | |
| if (!evaluatorForMatch) { | |
| return null; | |
| } | |
| if (!conditionMatch) { | |
| evaluatorForMatch = exactEvaluator; | |
| } else { | |
| evaluatorForMatch = EVALUATORS_BY_MATCH_TYPE[conditionMatch] || exactEvaluator; | |
| } |
Summary
custom_attribute_condition_evaluatormodule from JS to TS.Test plan