[ClientSide Routing] Launch v3 AB test / Simplify A/B testing #5099
Conversation
| setAliases({ | ||
| "express-http-context": path.resolve( |
There was a problem hiding this comment.
Since we're sharing context values across application boundaries, we need to be sure it still works while yarn linking. require-control forces the node process to look in a specific node_modules location for packages. Very useful lib that solved a lot of earlier yarn link problems.
damassi
force-pushed
the
add-express-context
branch
from
755b69f to
066565c
Compare
|
|
||
| const artworkSlug = location.pathname.replace(/\/artwork\//, "") | ||
| recordArtworkView(artworkSlug, sd.CURRENT_USER) | ||
| if (pageType === "artwork") { |
There was a problem hiding this comment.
Interesting, does this wind up firing as you navigate (to an artwork, when starting your session on a non-artwork page)?
Looks like the other things here (mediator.on hooks) just need to be called once in your session, in order to successfully be registered. Whereas this recordArtworkView should be called potentially on every navigation?
(sidenote- this is a mutation to record a view, firing on render. prob an oversight it's not in Reaction)
There was a problem hiding this comment.
This was how you had it before -- i had revised the inside of this in a previous PR and only mounted the client bits if it was an exactly page match on SSR but realized last night that we need this client stuff mounted immediately as we can navigate to the page later. So I just re-added this how it was.
| export const skipIfClientSideRoutingEnabled = (_req, _res, next) => { | ||
| if ( | ||
| getSplitTest("EXPERIMENTAL_APP_SHELL") || |
There was a problem hiding this comment.
Just to confirm, the DX flow is:
- be able to toggle feature completely on or off via feature flag
- ensure feature works totally on or off
- staging is generally enabled (always on)
- AB test value overrides feature flag value (and uses the new context lib)
| @@ -0,0 +1,10 @@ | |||
| import httpContext from "express-http-context" | |||
| @@ -208,6 +209,18 @@ export default function(app) { | |||
| ) | |||
| app.use("/(.well-known/)?apple-app-site-association", siteAssociation) | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
Thanks for the code comment. This feels like the right place stack-wise for this (after those middlewares that will mess this up, but before the actual AB middleware and app routes).
|
LGTM! Looking forward to playing with this. Echoing my comment from artsy/reaction#3191 (comment), namely, should we hold these PR's open until the Node v12 upgrade is fully deployed to prod? |
|
I'm personally not worried about Node 12 in prod -- the issue that we ran into before (seemed) to have been a bug in V8 JS engine that was fixed in node 10.4+. But yeah down for whatever ya'll think is best. |
…into damassi-add-express-context
|
Companion PR: artsy/reaction#3191 (needs to be merged first)
This sets up the A/B test for client-side routing as well as simplifies how we do A/B tests in the future across force / reaction boundaries via the use of express-http-context.
How it works:
res) object in order to grab the A/B test identifier (res.locals.sd.SOME_AB_TEST), we use a newsetSplitTest(ENV_VAR, value)utilgetSplitTest('ENV_VAR')getENV('ENV_VAR')util. We used this util before as a way to smooth over accessing environmental variables in an isomorphic way -- on the server, we grab fromprocess.env; on the client, fromsharify.express-http-context, keyed byENV_VARprocess.env. If not found there, the value is undefined.This simplifies things because we can build entire complex feature sets that can be turned on and off by ENV var, and then later, when an A/B test is needed to verify the feature, we don't need to change any code. It gives us an ENV-driven-development flow, where before we had to play a somewhat complex game of thread the needle, particularly on the server, and then go through an additional QA pass to ensure correctness.