fix: undefined is not a function

[hurali97]

Fixed Issues

$ Expensify/App#46117

Tests

1. What unit/integration tests cover your change? What autoQA tests cover your change?
   -- Added tests in Str-test.js
2. What tests did you perform that validates your changed worked?
   -- Applied changes to Expensify app and did normal usage of the App, since we don't have reproduction steps.

QA

1. What does QA need to do to validate your changes?
   -- Probably just normal usage of Expensify App
2. What areas to they need to test for regressions?
   N/A

[iwiznia]

Not sure why I got assigned instead of @francoisl but I have the same concern as him... this should not be needed and TS should be catching it. Let's keep the conversation in the issue though

[francoisl]

Based on this suggestion, let's also add a Log.warn in here or at least a console.warn?

[iwiznia]

Log please, adding conosle is useless as we don't have access to people's consoles.

[hurali97]

So I tried adding the Log.warn or Log.info but there were no logs in Expensify Web App console. There's this line in Log.jsx in expensify-common which has window.DEBUG:

expensify-common/lib/Log.jsx

Line 38 in 42663c3

isDebug: Utils.isWindowAvailable() ? window.DEBUG : false,

This window.DEBUG is undefined in the web app console when printed from node_modules/expensify-common/Log.js.

In Expensify codebase, we have explicitly marked isDebug: true here.

When I set isDebug: true in expensify-common/Log.jsx, I start getting logs in Expensify Web App as well.

Does anyone know if there's something I am missing with window.DEBUG while testing?

---

Once the above succeeds, I plan to do something like below in the App so we have logs in crashlytics as well, this is just an example:

diff --git a/src/libs/Log.ts b/src/libs/Log.ts
index 72673b8d3f..eb29b3ac08 100644
--- a/src/libs/Log.ts
+++ b/src/libs/Log.ts
@@ -13,6 +13,8 @@ import {shouldAttachLog} from './Console';
 import getPlatform from './getPlatform';
 import * as Network from './Network';
 import requireParameters from './requireParameters';

 let timeout: NodeJS.Timeout;
 let shouldCollectLogs = false;
@@ -73,6 +75,12 @@ const Log = new Logger({
 
         flushAllLogsOnAppLaunch().then(() => {
             console.debug(message, extraData);
+
+            if (message.indexOf('Str.getExtension') !== -1) {
+                crashlytics().log(message);
+                crashlytics().recordError(new Error(message, {cause: JSON.stringify(extraData)}));
+            }
+
             if (shouldCollectLogs) {
                 addLog({time: new Date(), level: CONST.DEBUG_CONSOLE.LEVELS.DEBUG, message, extraData});
             }

Let me know what you think of it 👍

[iwiznia]

Both Log.warn and Log.info should log in the console too... do you have all log levels enabled in the JS console?

[hurali97]

@iwiznia Yes I do have this enabled.

The issue is that window.DEBUG is undefined and as a result of which isDebug is set to false and we don't get any logs. Below is the diff from node_modules/expensify-common:

See diff

diff --git a/node_modules/expensify-common/dist/Log.js b/node_modules/expensify-common/dist/Log.js
index b7ffa9f..1202637 100644
--- a/node_modules/expensify-common/dist/Log.js
+++ b/node_modules/expensify-common/dist/Log.js
@@ -56,6 +56,8 @@ function clientLoggingCallback(message) {
         console.log(message);
     }
 }
+
+console.log('=== window.DEBUG ', window.DEBUG);
 exports.default = new Logger_1.default({
     serverLoggingCallback,
     clientLoggingCallback,
diff --git a/node_modules/expensify-common/dist/str.js b/node_modules/expensify-common/dist/str.js
index 2ebc6eb..3b2d83e 100644
--- a/node_modules/expensify-common/dist/str.js
+++ b/node_modules/expensify-common/dist/str.js
@@ -23,6 +23,9 @@ var __importStar = (this && this.__importStar) || function (mod) {
     __setModuleDefault(result, mod);
     return result;
 };
+var __importDefault = (this && this.__importDefault) || function (mod) {
+    return (mod && mod.__esModule) ? mod : { "default": mod };
+};
 Object.defineProperty(exports, "__esModule", { value: true });
 /* eslint-disable no-control-regex */
 const awesome_phonenumber_1 = require("awesome-phonenumber");
@@ -30,6 +33,7 @@ const HtmlEntities = __importStar(require("html-entities"));
 const Constants = __importStar(require("./CONST"));
 const UrlPatterns = __importStar(require("./Url"));
 const Utils = __importStar(require("./utils"));
+const Log_1 = __importDefault(require("./Log"));
 const REMOVE_SMS_DOMAIN_PATTERN = /@expensify\.sms/gi;
 function resultFn(parameter, ...args) {
     if (typeof parameter === 'function') {
@@ -909,6 +913,10 @@ const Str = {
      */
     getExtension(url) {
         var _a, _b;
+        if (typeof url !== 'string') {
+            Log_1.default.warn('=== Str.getExtension: url is not a string', { url });
+            return undefined;
+        }
         return (_b = (_a = url.split('.').pop()) === null || _a === void 0 ? void 0 : _a.split('?')[0]) === null || _b === void 0 ? void 0 : _b.toLowerCase();
     },
     /**

When I set isDebug: true directly in node_modules/expensify-common/dist/Log.js, I start to get logs. Below is the update:

//node_modules/expensify-common/dist/Log.js

console.log('=== window.DEBUG ', window.DEBUG);
exports.default = new Logger_1.default({
    serverLoggingCallback,
    clientLoggingCallback,
    isDebug: true// Utils.isWindowAvailable() ? window.DEBUG : false,
});

Log is visible in the console when isDebug: true:

---

I don't have any context on window.DEBUG, so it will be great if someone can share details on it and why it can be undefined.

[hurali97]

@francoisl So using Log.alert() appears to not be that helpful because it lacks the symbolication for the stack trace. I have recorded this by calling getExtension with numeric data in App.tsx while setting webpack to production mode in webpack.dev.ts.

Do you know it will show appropriate function names when used in actual prod?

[iwiznia]

Is that stack trace useful? It says webpack-internal in all of them....

[hurali97]

Yes that's my question as well. It seems not useful, so we can use Log.warn instead.

[francoisl]

Oh ok I see, it doesn't look useful indeed. Sounds good for Log.warn then.

[iwiznia]

My guess is that it won't make it easier for us to find where this is being called wrong.... but we'll see.

[francoisl]

Thanks!

[github-actions]

🚀Published to npm in v2.0.87