fix(node): Time zone handling for cron (getsentry#11225)

The `cron` package uses `timeZone`, while Sentry internally uses `timezone`. This caused Sentry cron jobs to be incorrectly upserted with no time zone information. Because of the use of the `...(timeZone ? { timeZone } : undefined)` idiom, TypeScript type checking did not catch the mistake. I kept `cron`'s `timeZone` capitalization within Sentry's instrumentation and changed from the `...(timeZone ? { timeZone } : undefined)` idiom so TypeScript could catch mistakes of this sort. (Passing an undefined `timezone` appears to be okay, because Sentry's `node-cron` instrumentation does it.)
cadesalaberry · Apr 19, 2024 · 0b08182 · 0b08182
1 parent 0791e50
commit 0b08182
Show file tree

Hide file tree

Showing 4 changed files with 32 additions and 18 deletions.
diff --git a/packages/node-experimental/src/cron/cron.ts b/packages/node-experimental/src/cron/cron.ts
@@ -100,7 +100,7 @@ export function instrumentCron<T>(lib: T & CronJobConstructor, monitorSlug: stri
           },
           {
             schedule: { type: 'crontab', value: cronString },
-            ...(timeZone ? { timeZone } : {}),
+            timezone: timeZone || undefined,
           },
         );
       }
@@ -132,7 +132,7 @@ export function instrumentCron<T>(lib: T & CronJobConstructor, monitorSlug: stri
               },
               {
                 schedule: { type: 'crontab', value: cronString },
-                ...(timeZone ? { timeZone } : {}),
+                timezone: timeZone || undefined,
               },
             );
           };

diff --git a/packages/node-experimental/test/cron.test.ts b/packages/node-experimental/test/cron.test.ts
@@ -53,13 +53,20 @@ describe('cron check-ins', () => {
 
       const CronJobWithCheckIn = cron.instrumentCron(CronJobMock, 'my-cron-job');
 
-      new CronJobWithCheckIn('* * * Jan,Sep Sun', () => {
-        expect(withMonitorSpy).toHaveBeenCalledTimes(1);
-        expect(withMonitorSpy).toHaveBeenLastCalledWith('my-cron-job', expect.anything(), {
-          schedule: { type: 'crontab', value: '* * * 1,9 0' },
-        });
-        done();
-      });
+      new CronJobWithCheckIn(
+        '* * * Jan,Sep Sun',
+        () => {
+          expect(withMonitorSpy).toHaveBeenCalledTimes(1);
+          expect(withMonitorSpy).toHaveBeenLastCalledWith('my-cron-job', expect.anything(), {
+            schedule: { type: 'crontab', value: '* * * 1,9 0' },
+            timezone: 'America/Los_Angeles',
+          });
+          done();
+        },
+        undefined,
+        true,
+        'America/Los_Angeles',
+      );
     });
 
     test('CronJob.from()', done => {

diff --git a/packages/node/src/cron/cron.ts b/packages/node/src/cron/cron.ts
@@ -100,7 +100,7 @@ export function instrumentCron<T>(lib: T & CronJobConstructor, monitorSlug: stri
           },
           {
             schedule: { type: 'crontab', value: cronString },
-            ...(timeZone ? { timeZone } : {}),
+            timezone: timeZone || undefined,
           },
         );
       }
@@ -132,7 +132,7 @@ export function instrumentCron<T>(lib: T & CronJobConstructor, monitorSlug: stri
               },
               {
                 schedule: { type: 'crontab', value: cronString },
-                ...(timeZone ? { timeZone } : {}),
+                timezone: timeZone || undefined,
               },
             );
           };

diff --git a/packages/node/test/cron.test.ts b/packages/node/test/cron.test.ts
@@ -53,13 +53,20 @@ describe('cron check-ins', () => {
 
       const CronJobWithCheckIn = cron.instrumentCron(CronJobMock, 'my-cron-job');
 
-      new CronJobWithCheckIn('* * * Jan,Sep Sun', () => {
-        expect(withMonitorSpy).toHaveBeenCalledTimes(1);
-        expect(withMonitorSpy).toHaveBeenLastCalledWith('my-cron-job', expect.anything(), {
-          schedule: { type: 'crontab', value: '* * * 1,9 0' },
-        });
-        done();
-      });
+      new CronJobWithCheckIn(
+        '* * * Jan,Sep Sun',
+        () => {
+          expect(withMonitorSpy).toHaveBeenCalledTimes(1);
+          expect(withMonitorSpy).toHaveBeenLastCalledWith('my-cron-job', expect.anything(), {
+            schedule: { type: 'crontab', value: '* * * 1,9 0' },
+            timezone: 'America/Los_Angeles',
+          });
+          done();
+        },
+        undefined,
+        true,
+        'America/Los_Angeles',
+      );
     });
 
     test('CronJob.from()', done => {