Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: undefined is not a function #795

Merged
merged 3 commits into from
Sep 10, 2024
Merged

fix: undefined is not a function #795

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d192a20
fix: add explicity check for typeof string to prevent undefined behav…
hurali97 Sep 2, 2024
94b31d3
test: add test cases for handling undefined behaviour of getExtension…
hurali97 Sep 2, 2024
01a0504
feat: add Log.warn in case of type mis match
hurali97 Sep 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions __tests__/Str-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,3 +215,19 @@ describe('Str.isValidE164Phone', () => {
expect(Str.isValidE164Phone('+14404589784')).toBeTruthy();
});
});

describe('Str.getExtension', () => {
it('Correctly returns extension', () => {
expect(Str.getExtension(buildTestURLForType('pdf'))).toBeTruthy();
expect(Str.getExtension(buildTestURLForType('PDF'))).toBeTruthy();
expect(Str.getExtension(buildTestURLForType('mov'))).toBeTruthy();
expect(Str.getExtension(buildTestURLForType('MP4'))).toBeTruthy();
});

it('Returns undefined for data types other than string', () => {
expect(Str.getExtension(0)).toBeUndefined();
expect(Str.getExtension(null)).toBeUndefined();
expect(Str.getExtension(undefined)).toBeUndefined();
expect(Str.getExtension([])).toBeUndefined();
});
});
1 change: 1 addition & 0 deletions lib/str.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -994,6 +994,7 @@ const Str = {
* without query parameters
*/
getExtension(url: string): string | undefined {
if (typeof url !== 'string') return undefined;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@hurali97 hurali97 Sep 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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:

Screenshot 2024-09-06 at 1 55 50 PM

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:

Screenshot 2024-09-06 at 1 56 26 PM

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Screenshot 2024-09-09 at 1 05 35 PM

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return url.split('.').pop()?.split('?')[0]?.toLowerCase();
},

Expand Down
Loading