config(): Add safety falback for when running in production #11686
Conversation
dmsnell
added
Framework
[Status] Needs Review
server/config/index.js
Outdated
| * to crash execution early. However, because many modules | ||
| * call this function in the module-global scope a failure | ||
| * here can not only crash that module but also entire | ||
| * subsystems within Calypso. Therefore if the NODE_ENV is |
There was a problem hiding this comment.
what does subsystem mean? Are there any subsystems other than the data-layer?
There was a problem hiding this comment.
the entire redux tree, sections, libs
can you think of a clearer description? I'm open to edits
There was a problem hiding this comment.
What if we simplify it a bit?
However, because many modules call this function in the module-global scope early in execution, this will likely break main application flows or cause unexpected behavior to occur.
There was a problem hiding this comment.
Updated wording in 3f64db7 (and rebased to squash)
samouri
dismissed
their stale review
I was wrong, config is actually imported within regenerate-client and its functions are toStringed
I think that this is an incorrect statement. I believe that I have changed the one that needs to be updated. |
|
Code + Behavior LGTM |
server/config/index.js
Outdated
| // display console error only in a browser | ||
| // (not in tests, for example) | ||
| if ( 'undefined' !== typeof window ) { | ||
| console.error( errorMessage ); |
There was a problem hiding this comment.
Wouldn't the above thrown error log to console? Or, if not, would it be enough to move console.log into the NODE_ENV condition (since tests have their own NODE_ENV)?
There was a problem hiding this comment.
not sure what you mean @aduth - they both log to the console but logging to the console isn't the problem. the thrown error halts the execution of the current JavaScript thread and that causes the big problem because it kills code which does nothing more than import the broken modules
it's my belief right now that we want to make an annoying console message so that any of us can quickly identify the failures in production but we want to prevent those failures from cascading outward thus removing the error
There was a problem hiding this comment.
it's my belief right now that we want to make an annoying console message so that any of us can quickly identify the failures in production but we want to prevent those failures from cascading outward thus removing the error
Makes sense 👍
There was a problem hiding this comment.
Should we add in some styling to differentiate our Calypso framework warnings from React errors? For example:
console.error( `%c ${ error Message }`, 'color:white; background:black;' );
There was a problem hiding this comment.
in f987c75 I added some styling @gwwar - wha'd'ya think?
Recently the data layer experienced a total failure when #11490 merged in and crashed when it failed to initialize the new middleware handler. The fact that a key was requested for which no value existed led to `config()` raising an `Error` which causes the entire middleware chain to fail to build. Now `config()` has been modified to only raise an actual `Error` when the `NODE_ENV=developement` and instead in all other environments to simply log a message to the browser console and return `undefined`. This should allow us to still catch any mistakes by the obvious message while preventing such otherwise-small errors from cascading out-of-control.
dmsnell
force-pushed
the
update/fail-safe-config
branch
from
b16c5a1 to
3f64db7
Compare
matticbot
added
[Size] L
Instead of showing the same message in the console as in the browser we can split the message into a styled and unstyled component. This commit does just that using visual styling in the in-browser error message to make it stand out among other warnings and messages.
dmsnell
force-pushed
the
update/fail-safe-config
branch
from
f987c75 to
9186e67
Compare
Yaaas! in the meantime I think we all need to start paying more attention to values assigned in the module scope that are runtime-dependent. |
matticbot
removed
the
[Status] Needs Review
Recently the data layer experienced a total failure when #11490 merged
in and crashed when it failed to initialize the new middleware handler.
The fact that a key was requested for which no value existed led to
config()raising anErrorwhich causes the entire middleware chainto fail to build.
Now
config()has been modified to only raise an actualErrorwhenthe
NODE_ENV=developementand instead in all other environments tosimply log a message to the browser console and return
undefined. Thisshould allow us to still catch any mistakes by the obvious message while
preventing such otherwise-small errors from cascading out-of-control.
Testing
Since no specific changes were made to app code a smoke-test and test-run should suffice to validate these changes.
config()is generated at build time so make sure to at leastmake buildbefore testing. There are new unit tests to confirm the behavior but this should get a manual confirmation as well.Console message for non-development environments
😄