Simplify A/B test lookup via express-http-context
#3191
Conversation
artsy-peril
bot
added
the
Version: Patch
damassi
force-pushed
the
wire-up-context
branch
from
e5c9c33 to
1699e4a
Compare
| let envVar | ||
| if (isServer) { | ||
| const httpContext = require("express-http-context") | ||
| envVar = httpContext.get(ENV_VAR) ?? process.env[ENV_VAR] |
There was a problem hiding this comment.
Uses new nullish coalesing plugin, which always returns the left hand side of the operation unless null or undefined.
There was a problem hiding this comment.
Interesting. I didn't fully get immediately how different is than ||. I do now recognize the subtle difference, but I think generally any sort of conditional assignment like this, if you are expecting a false-y/null/undefined to be possible (or even valid, esp. as a default), I almost always open a console to try it out (and/or mentally unpack it in my brain for a second).
Cool! I look forward to trying this.
| @@ -19,6 +19,8 @@ | |||
| } | |||
| ], | |||
| "@babel/plugin-proposal-class-properties", | |||
| "@babel/plugin-proposal-optional-chaining", | |||
There was a problem hiding this comment.
Suprised this wasn't already in! We can no do
const deepPropOrUndefined = foo?.bar?.baz?.bam
which should eliminate our need for the get util.
damassi
force-pushed
the
wire-up-context
branch
from
1699e4a to
0631c7d
Compare
| position: "absolute", | ||
| top: 0, | ||
| zIndex: 1100, // over top nav | ||
| }} |
There was a problem hiding this comment.
Fixes long standing issue where when a user views an artwork in full screen, the top nav bar flops out due to z-index and position.
| > | ||
| View all | ||
| </Button> | ||
| <RouterLink to={buttonHref}> |
There was a problem hiding this comment.
Ah cool, yea these custom window.location.href navigations in JS dont work with our router.
|
This looks good! Since this depends on Node v12, which still hasn't been fully deployed to production, so we hold off on merging these PR's until we're happy with v12 in prod? |
mzikherman
force-pushed
the
wire-up-context
branch
from
514fc17 to
a9447e6
Compare
Codecov Report
@@ Coverage Diff @@
## master #3191 +/- ##
========================================
+ Coverage 82.4% 82.4% +<.1%
========================================
Files 761 761
Lines 11357 11357
Branches 2245 2245
========================================
+ Hits 9360 9361 +1
+ Misses 1386 1385 -1
Partials 611 611 |
|
🚀 PR was released in |
See artsy/force#5099 for more info.
This updates our
getENVisomorphic ENV lookup util with another server-side layer:SOME_ENVvar exists within the request / response cycle, stored within express-http-context. If so, return that value. Useful for A/B testing.process.env.SOME_ENV