Handle failure in creating a state diff by logging (and crash-reporting) the full path of any keys that are null #13240
Conversation
…ng) the full path of any keys that are null. Fix #13239
Codecov Report
@@ Coverage Diff @@
## master #13240 +/- ##
==========================================
- Coverage 55.94% 55.93% -0.02%
==========================================
Files 281 281
Lines 27831 27873 +42
Branches 4563 4568 +5
==========================================
+ Hits 15570 15590 +20
- Misses 12261 12283 +22
|
Handle failure in creating a state diff by logging (and crash-reporting) the full path of any keys that are null
Handle failure in creating a state diff by logging (and crash-reporting) the full path of any keys that are null
| // one possible reason immutablediff can throw an error | ||
| // is due to null keys, so let's log any that we find | ||
| const nullKeyPaths = findNullKeyPaths(appState) | ||
| const error = (typeof e === 'object') |
There was a problem hiding this comment.
I think we should be more specific about the type of error before we do a null key check. We don't want to assume that all exceptions are this specific issue.
There was a problem hiding this comment.
The trouble is that immutablediff does not throw an error object, just a (dynamic) string. This is the only case I've seen immutablediff fail so far, and I did not want to go down the route of either:
- parsing the string, since it could change in a later immutablediff version
- assuming string vs Error object would mean a null-key error, since that could also change
Again, since the app will be unusable in this state, I wanted to err on the side of caution with regards to making sure we get the right error output and reported, so that we can solve the issue. But happy to take advice on this.
| nullKeys.push(keyPath) | ||
| } | ||
| // recursive, to find deep keys | ||
| nullKeys.push(...api.findNullKeyPaths(state.get(key), keyPath)) |
There was a problem hiding this comment.
recursion will be substantially more expensive than iteration to go through all the keys
There was a problem hiding this comment.
Is it worth spending the time to get this for...of...recurse loop into a flat iteration loop considering that if this code gets run the app is in an unusable state anyway? I do not believe it will further impact the user apart from making sure we get the correct error logged and therefore fixed more quickly.
There was a problem hiding this comment.
So... I was intrigued by the challenge, and tested what I came up with for a non-recursive function. It runs ~30% slower than the recursive one https://jsperf.com/immutable-find-null-paths-recursion-vs-iteration Perhaps someone can improve on it.
bsclifton
modified the milestones:
0.22.x (Developer Channel),
0.20.x Release 3 (Hotfix)
Handle failure in creating a state diff by logging (and crash-reporting) the full path of any keys that are null
|
0.20.x 2c3f975 |
Handle failure in creating a state diff by logging (and crash-reporting) the full path of any keys that are null
A null key path was fixed in #13234, but there may be other cases, so we should add better logging to the console and for the crash reports.
Performance should be unaffected since v8 can now optimize functions which contain a try...catch block.
Fix #13239
Submitter Checklist:
git rebase -ito squash commits (if needed).Test Plan:
This should cause no change in functionality, or extra log lines.
If a dev really wants to check the output in error conditions, they can force a null key path in the state, for example in appStore.js at the top of the
emitChangesfunction:appState = appState.setIn(['historySites', null], Immutable.fromJS({ hello: 'a' }))Reviewer Checklist:
Tests