feat (Multiple Route Matches): Allow multiple route matches via a shouldHandle property by jamesopti · Pull Request #12 · optimizely/nuclear-router

jamesopti · 2018-10-24T19:01:12Z

Summary

Enable multiple routes to match via the match regex property.
Use shouldHandle to break the tie.

shouldHandle is an optional property of a route which expects a function which returns a Promise
If that Promise resolves without explicitly passing false, the route will be considered a match.

The PromiseOrderedFirst utility is written to ensure deterministic behavior for a given set of matched routes regardless of how long any given match's shouldHandle promise takes to resolve.

Multiple Match Example Use Cases

Case 1 - All matches use shouldHandle

If data.type is basketball, the first route will be used
If data.type is football, the second route will be used
Else the catchall route will be used

{
    match: '/route123',
    shouldHandle: () => {
      return FetchData().then(data => data.type === 'basketball')
    },
    handle: [
      ...
    ]
},
{
    match: '/route123',
    shouldHandle: () => {
      return FetchData().then(data => data.type === 'football')
    },
    handle: [
      ...
    ]
}

Case 2 - Only some matches use shouldHandle

If data.type is basketball, the first route will be used
Else the second route will be used

{
    match: '/route123',
    shouldHandle: () => {
      return FetchData().then(data => data.type === 'basketball')
    },
    handle: [
      ...
    ]
},
{
    match: '/route123',
    handle: [
      ...
    ]
},

Case 3 - More than one shouldHandle matches

The first route will always be used.

{
    match: '/route123',
    handle: [
      ...
    ]
},
{
    match: '/route123',
    shouldHandle: () => {
      return FetchData().then(data => data.type === 'football')
    },
    handle: [
      ...
    ]
}

Test Plan

Unit tests added to cover the following scenarios:

List of route matches that look like:

[ <no shouldHandle>, <no shouldHandle>, ]
[ <shouldHandle rejected immediately>, <shouldHandle resolved immediately> ]
[ <shouldHandle rejected after delay>, <shouldHandle resolved immediately> ]
[ <shouldHandle resolved after delay>, <shouldHandle resolved immediately> ]

quintondang

thank you so much for doing this @jamesopti! Mostly super minor questions. overall really good

quintondang · 2018-10-24T21:41:55Z

+  return new Promise((res, rej) => {
+    if (routes.length === 0) {
+      rej('No routes match');
+    } else if (routes.length === 1) {


do we care about the case if someone accidentally adds a singular route with a shouldHandle fn? i.e. if they thought their route was a match but wasn't, or if they wanted to gate it anyways even if its a singular route. Otherwise this would fail silently and take all users to the route

Hmmmmm, thats an interesting catch!

I added this as a shortcut, but I think you're probably right.

shouldHandle should be respected as a secondary matching criteria if specified, even if there is only one match.

Will update.

quintondang · 2018-10-24T21:43:25Z

+        .catch(res => {
+          promiseToStateMap[index] = { state: "rejected", res, index };
+        })
+        .then(compute, compute);


we want to compute twice? or am I misunderstanding this

No, this just means we invoke compute on resolve or on reject of the promise.

Signature: .then(<onFulfilled>, <onRejected>)

This is a safer alternative/fallback to .finally(<onFulfilled or rejected>) because .finally is only supported starting in Chrome 63.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/finally

ohhh right thanks for the explanation! Did not realize that about finally.

quintondang · 2018-10-24T21:53:50Z

+ * This is useful to control test timing where our
+ * router resolves routes using Promises (micro-task timing)
+ * More on micro-tasks vs macro-tasks:
+ *   https://jakearchibald.com/2015/tasks-microtasks-queues-and-schedules/


very interesting read thanks!

NP! See here for where Nikhil and I learned about this timing behavior.

https://github.com/optimizely/client-js/blob/devel/src/plugins/change_attribute/index.js#L175-L196

quintondang · 2018-10-24T21:55:18Z

+          ]);
+        });
+
+        it('should execute the first route in the list', () => {


update this to second route in the list

quintondang · 2018-10-24T21:56:07Z

+            .then(() => {
+              sinon.assert.calledOnce(handlerSpy2)
+              sinon.assert.notCalled(handlerSpy1)
+              sinon.assert.callOrder(shouldHandleSpy2, shouldHandleSpy1, handlerSpy2)


nice! didn't know about callOrder

quintondang · 2018-10-24T21:57:29Z

+        .then(() => {
+          sinon.assert.calledOnce(onRouteCompleteSpy)
+          sinon.assert.callOrder(spy1, spy2, onRouteCompleteSpy)
+          var args = onRouteCompleteSpy.firstCall.args[0]


super nit: update these vars to consts but wont block on this

jamesopti

Thanks for the quick review @quintondang !

Updated for the single route case and added some more tests.

jamesopti · 2018-10-24T22:02:31Z

+  return new Promise((res, rej) => {
+    if (routes.length === 0) {
+      rej('No routes match');
+    } else if (routes.length === 1) {


Hmmmmm, thats an interesting catch!

I added this as a shortcut, but I think you're probably right.

shouldHandle should be respected as a secondary matching criteria if specified, even if there is only one match.

Will update.

jamesopti · 2018-10-24T22:04:39Z

+        .catch(res => {
+          promiseToStateMap[index] = { state: "rejected", res, index };
+        })
+        .then(compute, compute);


No, this just means we invoke compute on resolve or on reject of the promise.

Signature: .then(<onFulfilled>, <onRejected>)

This is a safer alternative/fallback to .finally(<onFulfilled or rejected>) because .finally is only supported starting in Chrome 63.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/finally

jamesopti · 2018-10-24T22:11:02Z

+ * This is useful to control test timing where our
+ * router resolves routes using Promises (micro-task timing)
+ * More on micro-tasks vs macro-tasks:
+ *   https://jakearchibald.com/2015/tasks-microtasks-queues-and-schedules/


NP! See here for where Nikhil and I learned about this timing behavior.

https://github.com/optimizely/client-js/blob/devel/src/plugins/change_attribute/index.js#L175-L196

quintondang

lgtm!! one smal question

quintondang · 2018-10-25T17:41:34Z

-      ).then(({index}) => res(routes[index]));
-    }
-  });
+  return utils


will this rej if no routes match?

Yup. Theres a test for that too.

PromiseOrderedFirst will iterate through the (empty) list of promises and since it wont find any pending or resolved ones, it will reject on first attempt.

jamesopti force-pushed the james/multiple_matching_routes branch 2 times, most recently from bcd1674 to c158f42 Compare October 24, 2018 19:32

allow multiple route matches via a shouldHandle property

f97b438

jamesopti force-pushed the james/multiple_matching_routes branch from c158f42 to f97b438 Compare October 24, 2018 20:08

jamesopti requested a review from a team as a code owner October 24, 2018 20:08

optimizely-codereview-bot added the 🤖 1 hrs label Oct 24, 2018

jamesopti assigned jamesopti and unassigned jamesopti Oct 24, 2018

optimizely-codereview-bot requested a review from quintondang October 24, 2018 21:11

quintondang suggested changes Oct 24, 2018

View reviewed changes

optimizely-codereview-bot removed the 🤖 1 hrs label Oct 24, 2018

optimizely-codereview-bot assigned jamesopti Oct 24, 2018

jamesopti commented Oct 24, 2018

View reviewed changes

jamesopti force-pushed the james/multiple_matching_routes branch from 0c65ead to 443d5a5 Compare October 24, 2018 23:07

cleanup and respond to PR feedback

d40ae0d

jamesopti force-pushed the james/multiple_matching_routes branch from 443d5a5 to d40ae0d Compare October 24, 2018 23:14

jamesopti removed their assignment Oct 24, 2018

jamesopti requested a review from quintondang October 25, 2018 01:23

cleanup filterMatches

1284530

optimizely-codereview-bot added 🤖 1 hrs 🤖 2 hrs and removed 🤖 1 hrs labels Oct 25, 2018

quintondang approved these changes Oct 25, 2018

View reviewed changes

jamesopti merged commit 6ddb4ab into master Oct 25, 2018

Conversation

jamesopti commented Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Multiple Match Example Use Cases

Test Plan

Uh oh!

quintondang left a comment

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesopti left a comment

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!

Choose a reason for hiding this comment

Uh oh!

quintondang left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jamesopti commented Oct 24, 2018 •

edited

Loading