DO NOT REVIEW - SEE https://github.com/optimizely/javascript-sdk/pull/288 by jamesopti · Pull Request #216 · optimizely/javascript-sdk

jamesopti · 2019-02-12T21:48:23Z

Summary

This PR adds the ability for an SDK consumer to pass their own set of condition evaluators when creating an optimizely instance.

The core motivator for this is to be able to evaluate WEB audience conditions using the Javascript SDK as part of the EDGE project (go/edge).

Future fullstack use cases for this functionality might include audience condition plugins, where the customer would define their own match types and provide their own matchers.

/**
 * Condition evaluators for conditions that can be evaluated in a Cloudflare worker using
 * information like cookies, query parameters, userAgent, etc...
 */
function getConditionEvaluators(inputs) {
  return {
    cookies: {
      evaluate: matchCookies.match.bind(this, { cookies: inputs.cookies }),
    },
    device: {
      evaluate: matchDevice.match.bind(this, { device: Detect.parseUA(inputs.ua).device }),
    },
    ...
    /* Other custom condition evaluators */
  }
}


var optlyInstance = optimizely.createInstance({
  datafile: datafile,
  conditionEvaluators: getConditionEvaluators(inputs),
  skipJSONValidation: true
});

Test plan

Unit tests would be pretty straightforward to add for this

Issues

packages/optimizely-sdk/lib/utils/enums/index.js

packages/optimizely-sdk/lib/core/audience_evaluator/index.js

nchilada · 2019-02-12T23:01:27Z

packages/optimizely-sdk/lib/core/audience_evaluator/index.js

+function AudienceEvaluator(conditionEvaluators) {
+  this.typeToEvaluatorMap = Object.assign({
+    'custom_attribute': customAttributeConditionEvaluator
+  }, conditionEvaluators);


Do we just want allow for additional condition types beyond those supported by the javascript-sdk, or do we also want to allow customers to modify how the built-in condition types are evaluated?

If the former, I wonder if we can add some kind of namespacing (as in Optimizely Web audience integrations) to protect custom conditions types from conflicting with (existing and future) built-in condition types.

If the latter, I have serious reservations, since that could make it difficult for us to modify our own condition types without affecting existing customers.

Yikes, we don't seem to catch errors when constructing the decision service. So if we apply any validation at this stage, or do anything that could throw an error, that error will bubble up into the customer's application.

Maybe we should do something about that! Though for the moment, I'm just pointing this out so that we remain careful not to throw.

I like the approach of not allowing the built in evaluators to be overridden. If you need to do that, then you'll be able to write your own.

Good call on the error catching. Added an error message for that which is used in a try/catch now.

packages/optimizely-sdk/lib/core/audience_evaluator/index.js

nchilada · 2019-02-12T23:05:38Z

packages/optimizely-sdk/lib/core/audience_evaluator/index.js

+function AudienceEvaluator(conditionEvaluators) {
+  this.typeToEvaluatorMap = Object.assign({
+    'custom_attribute': customAttributeConditionEvaluator
+  }, conditionEvaluators);


TODO: additional unit tests, eventually

packages/optimizely-sdk/lib/core/audience_evaluator/index.js

coveralls · 2019-02-12T23:36:09Z

Coverage decreased (-0.05%) to 97.534% when pulling 2ae49e1 on james/custom_condition_evaluators into de83b9b on master.

mikeproeng37 · 2019-02-21T23:31:09Z

Unit tests aren't passing in Travis, might wanna fix that and also merge master in. I will be reviewing this PR in the meantime!

mikeproeng37

I think the approach is fine so far. CCing @mjc1283 and @jordangarcia who have been doing work on extensibility and new components.

mikeproeng37 · 2019-02-21T23:35:49Z

packages/optimizely-sdk/lib/core/audience_evaluator/index.js

+    return null;
+  }
+  try {
+    return evaluator.evaluate(condition, userAttributes, this.logger);


if the evaluator we get is not the custom_attribute, we should log a debug message to let the user know that. Might be helpful when trying to debug. Maybe we can even just log which evaluator we ended using (by key)

packages/optimizely-sdk/lib/core/audience_evaluator/index.js

mikeproeng37 · 2019-02-21T23:38:34Z

packages/optimizely-sdk/lib/core/decision_service/index.js

  this.configObj = options.configObj;
  this.userProfileService = options.userProfileService || null;
  this.logger = options.logger;
+  this.audienceEvaluator = new AudienceEvaluator(options.logger, options.__exploratoryConditionEvaluators);


This feels like we are polluting our factory method haha but I see no other easy way to integrate into the current setup.

You mean the constructor?

Yea in the current setup, all this state must be threaded through (config, logger instance, etc...)

Would be nice to have a global registration system by instance ID so that this kinda global state can be easily accessed.

This is totally fine, by adding configuration to the AudienceEvaluator we must capture that state, so making this an instantiatable class makes sense.

As far as global state, things like audience evaluators should not be considered as global state. If some sort of global registry existed you'd still have to know the instance ID here to resolve the audience evaluators (which would have to be threaded through).

However things like the logger and errorHandler should be global and having to thread them through here is not cool (this is changing soon in another PR)

jordangarcia

A few minor things about naming, but I think the structure of how the evaluator was made extensible looks good.

In terms of naming I've seen React use the prefix UNSTABLE_ in the public API and think that probably gets the point across more the "exploratory".

packages/optimizely-sdk/lib/core/audience_evaluator/index.js

jordangarcia · 2019-02-26T16:56:59Z

packages/optimizely-sdk/lib/optimizely/index.tests.js

            configObj: optlyInstance.configObj,
            userProfileService: userProfileServiceInstance,
            logger: createdLogger,
+            __exploratoryConditionEvaluators: undefined


This is kind of a weird test assertion. It's probably fine, but I also think it makes sense for __exporatoryConditionEvaluators to be required in new Optimizely or Optimizely defaults this to {}

jordangarcia · 2019-02-26T16:59:23Z

packages/optimizely-sdk/lib/core/decision_service/index.js

  this.configObj = options.configObj;
  this.userProfileService = options.userProfileService || null;
  this.logger = options.logger;
+  this.audienceEvaluator = new AudienceEvaluator(options.logger, options.__exploratoryConditionEvaluators);


This is totally fine, by adding configuration to the AudienceEvaluator we must capture that state, so making this an instantiatable class makes sense.

As far as global state, things like audience evaluators should not be considered as global state. If some sort of global registry existed you'd still have to know the instance ID here to resolve the audience evaluators (which would have to be threaded through).

However things like the logger and errorHandler should be global and having to thread them through here is not cool (this is changing soon in another PR)

lpappone · 2019-06-06T03:25:58Z

packages/optimizely-sdk/lib/core/custom_attribute_condition_evaluator/index.js

 */
 function evaluate(condition, userAttributes, logger) {
  if (condition.type !== CUSTOM_ATTRIBUTE_CONDITION_TYPE) {
-    logger.log(LOG_LEVEL.WARNING, sprintf(LOG_MESSAGES.UNKNOWN_CONDITION_TYPE, MODULE_NAME, JSON.stringify(condition)));


@jamesopti I see this log message now in AudienceEvaluator.prototype.evaluateConditionWithUserAttributes - what is the purpose of this evaluate function, now?

lpappone · 2019-06-07T22:11:55Z

DO NOT REVIEW - SEE #288

mikeproeng37 · 2019-07-24T17:37:15Z

Obsolete

jamesopti added the do-not-review label Feb 12, 2019

James Fox added 2 commits February 12, 2019 14:04

add the ability to provide custom condition evaluators

0738b58

rebase with latest audience editor changes

2ae49e1

jamesopti force-pushed the james/custom_condition_evaluators branch from 6c02325 to 2ae49e1 Compare February 12, 2019 22:11

nchilada suggested changes Feb 12, 2019

View reviewed changes

packages/optimizely-sdk/lib/utils/enums/index.js Outdated Show resolved Hide resolved

packages/optimizely-sdk/lib/core/audience_evaluator/index.js Show resolved Hide resolved

nchilada suggested changes Feb 12, 2019

View reviewed changes

updates per code review

0112ff9

mikeproeng37 reviewed Feb 21, 2019

View reviewed changes

mikeproeng37 requested a review from mjc1283 February 21, 2019 23:39

jordangarcia suggested changes Feb 26, 2019

View reviewed changes

lpappone added 3 commits May 30, 2019 16:33

fix merge conflicts

1ce7345

fix lint command

930975c

update name

db29de1

lpappone reviewed Jun 6, 2019

View reviewed changes

lpappone added 8 commits June 5, 2019 20:30

fix tests

7411b24

noop to rebuild

1102191

remove quotes

05deac9

add quotes back in

76d155e

regenerate packagelock.json

a023b08

regenerate package-lock.json again

0586dd8

regenerate another package-lock.json again

f97df78

copy pasta

53f43a6

lpappone changed the title ~~feat (AudienceEvaluator): Add the ability to provide custom condition evaluators~~ DO NOT REVIEW - SEE https://github.com/optimizely/javascript-sdk/pull/288 Jun 7, 2019

jamesopti mentioned this pull request Jun 7, 2019

feat(AudienceEvaluator): Add the ability to provide custom condition evaluators #288

Merged

mikeproeng37 closed this Jul 24, 2019

mikeproeng37 deleted the james/custom_condition_evaluators branch September 23, 2019 22:12

Conversation

jamesopti commented Feb 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Issues

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

coveralls commented Feb 12, 2019

Uh oh!

mikeproeng37 commented Feb 21, 2019

Uh oh!

mikeproeng37 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jordangarcia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpappone commented Jun 7, 2019

Uh oh!

mikeproeng37 commented Jul 24, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Comments

jamesopti commented Feb 12, 2019 •

edited

Loading