Add missing default value to extras
#696
Conversation
lib/ExpensiMark.js
Outdated
| // eslint-disable-next-line no-console | ||
| console.warn(`[ExpensiMark] Missing report name for ID: ${ g1}`); |
There was a problem hiding this comment.
Oh, this is not what I meant, we don't use console like this (as the linter is telling you). You should log using the Log class. Also please change them all to alert instead of warn.
There was a problem hiding this comment.
In other places in ExpensiMark.js there is no Log class usages - only console.warn is used twice. That's why we have done it this way. I can change it though if you think that Log is more suitable in this case
There was a problem hiding this comment.
Actually, we use. In expensimark there are some console.warn s. That's why I've used it. Can change if you want :)
There was a problem hiding this comment.
hmmmm not sure why we do that, but in this case, we want the log to go to our servers or we will never know it happened, since we don't have access to user's consoles.
There was a problem hiding this comment.
I've checked quickly and I don't see any usage of Log from expensify-common in E/App as well . Can we for now stay with the console.warn and check it as followup so we will fix the issue with missing extras quicker?
There was a problem hiding this comment.
We might not be using it yet, but it exists, no? Is it causing any problems if you use it? Not sure I understand the pushback...
There was a problem hiding this comment.
I just don't want to introduce any regression as Log is not yet used in ExpensiMark.
It's not a problem to change it, let me check more extensively in E/App. I'll ping you once I'll do it.
There was a problem hiding this comment.
Seems to work fine, but tests are failing - I'm on it right now
There was a problem hiding this comment.
@iwiznia I've pushed remaining changes. Tests were failing due to wrong test environment (as now we need window to be defined - this is used in log implementation)
lib/ExpensiMark.js
Outdated
| @@ -473,7 +474,8 @@ export default class ExpensiMark { | |||
| replacement: (match, g1, offset, string, extras) => { | |||
| const reportToNameMap = extras.reportIdToName; | |||
| if (!reportToNameMap || !reportToNameMap[g1]) { | |||
| return ''; | |||
| Log.alert(`[ExpensiMark] Missing report name for ID: ${g1}`); | |||
There was a problem hiding this comment.
Variable parameters should go in the 2nd parameter of the call, so:
| Log.alert(`[ExpensiMark] Missing report name for ID: ${g1}`); | |
| Log.alert('[ExpensiMark] Missing report name', {reportID: g1}); |
Fixed Issues
42161
Tests
QA