-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update expensify common with new logging methods and improve logging #4191
Changes from 10 commits
002bfef
2aa3b90
4a61d6f
1cbe7c0
96c962f
88884f0
fb6660e
fe3f578
8be321d
b8fe763
20fbc21
57d6d62
053b75d
04287bb
85f7ad6
124aade
f42e8db
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,7 @@ import Log from '../../libs/Log'; | |
|
||
BaseErrorBoundary.defaultProps.logError = (errorMessage, error, errorInfo) => { | ||
// Log the error to the server | ||
Log.alert(errorMessage, 0, {error: error.message, errorInfo}, false); | ||
Log.alert(errorMessage, {error: error.message, errorInfo}, false); | ||
}; | ||
|
||
window.Log = Log; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove? |
||
export default BaseErrorBoundary; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,8 @@ import _ from 'underscore'; | |
import CONFIG from '../CONFIG'; | ||
import CONST from '../CONST'; | ||
|
||
let info = () => {}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a comment? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
|
||
/** | ||
* Send an HTTP request, and attempt to resolve the json response. | ||
* If there is a network error, we'll set the application offline. | ||
|
@@ -28,10 +30,30 @@ function processHTTPRequest(url, method = 'get', body = null) { | |
* @returns {Promise} | ||
*/ | ||
function xhr(command, data, type = CONST.NETWORK.METHOD.POST, shouldUseSecure = false) { | ||
if (command !== 'Log') { | ||
marcaaron marked this conversation as resolved.
Show resolved
Hide resolved
|
||
info('Making API request', false, { | ||
marcaaron marked this conversation as resolved.
Show resolved
Hide resolved
|
||
command, | ||
type, | ||
shouldUseSecure, | ||
rvl: data.returnValueList, | ||
}); | ||
} | ||
marcaaron marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const formData = new FormData(); | ||
_.each(data, (val, key) => formData.append(key, val)); | ||
const apiRoot = shouldUseSecure ? CONFIG.EXPENSIFY.URL_EXPENSIFY_SECURE : CONFIG.EXPENSIFY.URL_API_ROOT; | ||
return processHTTPRequest(`${apiRoot}api?command=${command}`, type, formData); | ||
return processHTTPRequest(`${apiRoot}api?command=${command}`, type, formData) | ||
.then((response) => { | ||
if (command !== 'Log') { | ||
info('Finished API request', false, { | ||
command, | ||
type, | ||
shouldUseSecure, | ||
jsonCode: response.jsonCode, | ||
requestID: response.requestID, | ||
}); | ||
} | ||
return response; | ||
}); | ||
} | ||
|
||
/** | ||
|
@@ -51,7 +73,12 @@ function download(relativePath) { | |
return processHTTPRequest(`${siteRoot}${strippedRelativePath}`); | ||
} | ||
|
||
function setLogger(logger) { | ||
info = logger.info; | ||
} | ||
|
||
export default { | ||
setLogger, | ||
download, | ||
xhr, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ import {StackActions, DrawerActions} from '@react-navigation/native'; | |
import PropTypes from 'prop-types'; | ||
import Onyx from 'react-native-onyx'; | ||
import linkTo from './linkTo'; | ||
import Log from '../Log'; | ||
import ROUTES from '../../ROUTES'; | ||
import SCREENS from '../../SCREENS'; | ||
import CustomActions from './CustomActions'; | ||
|
@@ -54,6 +55,7 @@ function goBack(shouldOpenDrawer = true) { | |
* @param {String} route | ||
*/ | ||
function navigate(route = ROUTES.HOME) { | ||
Log.info('Navigating to route', false, {route}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NAB. Nice addition of this log! It might be even better to add a logger when the navigation state changes. Which will also capture things like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, I would need to add the log in this event right (and remove the logging from here)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, I think the "currentURL" or "path" is probably the most valuable to have now and moving the log there should capture everything and not just when we explicit use of There's also some other information in the "state" besides the path that could be useful to log - but less sure about that - more info here if you are curious. |
||
if (route === ROUTES.HOME) { | ||
if (isLoggedIn) { | ||
openDrawer(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heads up this logic was moved to another file which is why it's showing as an addition. Probably we don't want to call it in both places.
App/src/components/OnyxProvider.js
Lines 11 to 14 in 9368288
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh... that's why I had to re-add the imports 😄
Removing.