From 10be599fb86789046128f7e6924948ed58fdd3fa Mon Sep 17 00:00:00 2001 From: "Matthew Riley MacPherson (tofumatt)" Date: Thu, 3 Oct 2024 19:08:07 +0100 Subject: [PATCH 1/3] Update date ESLint rule to flag usage of `new Date()` with arguments. --- .eslintrc.json | 11 ++++++ assets/js/components/GoogleChart/index.js | 4 ++ .../notifications/BannerNotification/index.js | 2 + assets/js/modules/adsense/util/data-mock.js | 5 +++ .../modules/adsense/util/site-stats-data.js | 2 + .../UserCountGraph.js | 2 + .../custom-dimensions-gathering-data.js | 2 + .../analytics-4/datastore/partial-data.js | 2 + .../components/common/AnalyticsStats.js | 2 + .../modules/search-console/util/data-mock.js | 5 +++ assets/js/util/convert-time.js | 4 +- assets/js/util/dates.js | 6 +++ packages/eslint-plugin/configs/main.js | 12 +----- .../eslint-plugin/rules/no-direct-date.js | 7 +++- .../rules/no-direct-date.test.js | 38 ++++++++++++++++++- 15 files changed, 89 insertions(+), 15 deletions(-) diff --git a/.eslintrc.json b/.eslintrc.json index 3f272dbf6d2..d93d15e0dcb 100644 --- a/.eslintrc.json +++ b/.eslintrc.json @@ -169,6 +169,17 @@ "off" ] } + }, + { + "files": [ + "webpack/*", + "**/__factories__/*", + "*.stories.js", + "*.test.js" + ], + "rules": { + "sitekit/no-direct-date": "off" + } } ], "plugins": [ diff --git a/assets/js/components/GoogleChart/index.js b/assets/js/components/GoogleChart/index.js index 111e614a789..5f25106e21e 100644 --- a/assets/js/components/GoogleChart/index.js +++ b/assets/js/components/GoogleChart/index.js @@ -234,6 +234,8 @@ export default function GoogleChart( props ) { // Only use markers if the date is within the current date range. const dateMarkersInRange = dateMarkers.filter( ( dateMarker ) => { + // Valid use of `new Date()` with an argument. + // eslint-disable-next-line sitekit/no-direct-date return isDateWithinRange( new Date( dateMarker.date ) ); } ); @@ -260,6 +262,8 @@ export default function GoogleChart( props ) { // Add the dotted line and tooltip for each date marker. dateMarkersInRange.forEach( ( dateMarker, index ) => { + // Valid use of `new Date()` with an argument. + // eslint-disable-next-line sitekit/no-direct-date const dateFromMarker = new Date( dateMarker.date ); const chartLine = document.getElementById( diff --git a/assets/js/components/notifications/BannerNotification/index.js b/assets/js/components/notifications/BannerNotification/index.js index fc95d739d74..ab6df1fa4be 100644 --- a/assets/js/components/notifications/BannerNotification/index.js +++ b/assets/js/components/notifications/BannerNotification/index.js @@ -218,6 +218,8 @@ export default function BannerNotification( props ) { const { value: dismissed } = await getItem( cacheKeyDismissed ); if ( dismissed ) { + // Valid use of `new Date()` with an argument. + // eslint-disable-next-line sitekit/no-direct-date const expiration = new Date( dismissed ); expiration.setSeconds( diff --git a/assets/js/modules/adsense/util/data-mock.js b/assets/js/modules/adsense/util/data-mock.js index 1963c21fa2a..119fc3bbbab 100644 --- a/assets/js/modules/adsense/util/data-mock.js +++ b/assets/js/modules/adsense/util/data-mock.js @@ -257,9 +257,14 @@ export function getAdSenseMockResponse( args ) { const ops = [ // Converts range number to a date string. map( ( item ) => { + // Valid use of `new Date()` with an argument. + // eslint-disable-next-line sitekit/no-direct-date const updatedMilliseconds = new Date( startDate ).setDate( startDate.getDate() + item ); + + // Valid use of `new Date()` with an argument. + // eslint-disable-next-line sitekit/no-direct-date return getDateString( new Date( updatedMilliseconds ) ); } ), // Add dimension and metric values. diff --git a/assets/js/modules/adsense/util/site-stats-data.js b/assets/js/modules/adsense/util/site-stats-data.js index 50df6a61cc6..8fb1fb54e40 100644 --- a/assets/js/modules/adsense/util/site-stats-data.js +++ b/assets/js/modules/adsense/util/site-stats-data.js @@ -71,6 +71,8 @@ export function getSiteStatsDataForGoogleChart( ]; const nextDate = ( date ) => { + // Valid use of `new Date()` with an argument. + // eslint-disable-next-line sitekit/no-direct-date const next = new Date( date ); next.setDate( date.getDate() + 1 ); return next; diff --git a/assets/js/modules/analytics-4/components/dashboard/DashboardAllTrafficWidgetGA4/UserCountGraph.js b/assets/js/modules/analytics-4/components/dashboard/DashboardAllTrafficWidgetGA4/UserCountGraph.js index a1569f72bb1..6d43119fedf 100644 --- a/assets/js/modules/analytics-4/components/dashboard/DashboardAllTrafficWidgetGA4/UserCountGraph.js +++ b/assets/js/modules/analytics-4/components/dashboard/DashboardAllTrafficWidgetGA4/UserCountGraph.js @@ -145,6 +145,8 @@ export default function UserCountGraph( props ) { ? [ { date: getDateString( + // Valid use of `new Date()` with an argument. + // eslint-disable-next-line sitekit/no-direct-date new Date( propertyCreateTime ) ), text: __( diff --git a/assets/js/modules/analytics-4/datastore/custom-dimensions-gathering-data.js b/assets/js/modules/analytics-4/datastore/custom-dimensions-gathering-data.js index 7dd535bf7cf..f345f91364d 100644 --- a/assets/js/modules/analytics-4/datastore/custom-dimensions-gathering-data.js +++ b/assets/js/modules/analytics-4/datastore/custom-dimensions-gathering-data.js @@ -326,6 +326,8 @@ const baseSelectors = { const endDate = select( CORE_USER ).getReferenceDate(); return { + // Valid use of `new Date()` with an argument. + // eslint-disable-next-line sitekit/no-direct-date startDate: getDateString( new Date( propertyCreateTime ) ), endDate, dimensions: [ `customEvent:${ customDimension }` ], diff --git a/assets/js/modules/analytics-4/datastore/partial-data.js b/assets/js/modules/analytics-4/datastore/partial-data.js index f72853a9c0a..421956444f9 100644 --- a/assets/js/modules/analytics-4/datastore/partial-data.js +++ b/assets/js/modules/analytics-4/datastore/partial-data.js @@ -506,6 +506,8 @@ const baseSelectors = { return undefined; } + // Valid use of `new Date()` with an argument. + // eslint-disable-next-line sitekit/no-direct-date const startDate = getDateString( new Date( propertyCreateTime ) ); const endDate = select( CORE_USER ).getReferenceDate(); diff --git a/assets/js/modules/search-console/components/common/AnalyticsStats.js b/assets/js/modules/search-console/components/common/AnalyticsStats.js index d8579e91f95..36f72ceb54f 100644 --- a/assets/js/modules/search-console/components/common/AnalyticsStats.js +++ b/assets/js/modules/search-console/components/common/AnalyticsStats.js @@ -77,6 +77,8 @@ export default function AnalyticsStats( props ) { if ( propertyCreateTime ) { dateMarkers = [ { + // Valid use of `new Date()` with an argument. + // eslint-disable-next-line sitekit/no-direct-date date: getDateString( new Date( propertyCreateTime ) ), text: __( 'Google Analytics property created', diff --git a/assets/js/modules/search-console/util/data-mock.js b/assets/js/modules/search-console/util/data-mock.js index c0d059c4d07..8cc6facb487 100644 --- a/assets/js/modules/search-console/util/data-mock.js +++ b/assets/js/modules/search-console/util/data-mock.js @@ -78,9 +78,14 @@ export function getSearchConsoleMockResponse( args ) { const ops = [ // Converts range number to a date string. map( ( item ) => { + // Valid use of `new Date()` with an argument. + // eslint-disable-next-line sitekit/no-direct-date const updatedMilliseconds = new Date( startDate ).setDate( startDate.getDate() + item ); + + // Valid use of `new Date()` with an argument. + // eslint-disable-next-line sitekit/no-direct-date return getDateString( new Date( updatedMilliseconds ) ); } ), // Add dimension and metric values. diff --git a/assets/js/util/convert-time.js b/assets/js/util/convert-time.js index 4c9d99540d7..049d7812289 100644 --- a/assets/js/util/convert-time.js +++ b/assets/js/util/convert-time.js @@ -58,7 +58,9 @@ export const convertSecondsToArray = ( seconds ) => { export const convertDateStringToUNIXTimestamp = ( dateStringValue ) => { const unixTimestamp = dateStringValue && ! Number.isInteger( dateStringValue ) - ? new Date( dateStringValue ).getTime() + ? // Valid use of `new Date()` with an argument. + // eslint-disable-next-line sitekit/no-direct-date + new Date( dateStringValue ).getTime() : dateStringValue; if ( isNaN( unixTimestamp ) || ! unixTimestamp ) { diff --git a/assets/js/util/dates.js b/assets/js/util/dates.js index ef8295b7d70..5976a7ab3a2 100644 --- a/assets/js/util/dates.js +++ b/assets/js/util/dates.js @@ -114,7 +114,10 @@ export function isValidDateString( dateString = '' ) { return false; } + // Valid use of `new Date()`, constructing a new date from the string. + // eslint-disable-next-line sitekit/no-direct-date const date = new Date( dateString ); + return isDate( date ) && ! isNaN( date ); } @@ -217,5 +220,8 @@ export function dateSub( relativeDate, duration ) { const timestamp = isValidDateString( relativeDate ) ? Date.parse( relativeDate ) : relativeDate.getTime(); + + // Valid use of `new Date()` using calculations. + // eslint-disable-next-line sitekit/no-direct-date return new Date( timestamp - duration * 1000 ); } diff --git a/packages/eslint-plugin/configs/main.js b/packages/eslint-plugin/configs/main.js index 5fd51abd1f2..db76185e27f 100644 --- a/packages/eslint-plugin/configs/main.js +++ b/packages/eslint-plugin/configs/main.js @@ -27,16 +27,6 @@ module.exports = { 'sitekit/jsdoc-tag-grouping': [ 'error' ], 'sitekit/jsdoc-tag-order': [ 'error' ], 'sitekit/no-yield-dispatch': [ 'error' ], - 'sitekit/no-direct-date': [ - 'error', - { - ignoreFiles: [ - '*/webpack/*', - '*/__factories__/*', - '*.stories.js', - '*.test.js', - ], - }, - ], + 'sitekit/no-direct-date': [ 'error' ], }, }; diff --git a/packages/eslint-plugin/rules/no-direct-date.js b/packages/eslint-plugin/rules/no-direct-date.js index ed8b4a034ec..5defcecef87 100644 --- a/packages/eslint-plugin/rules/no-direct-date.js +++ b/packages/eslint-plugin/rules/no-direct-date.js @@ -53,7 +53,8 @@ module.exports = { NewExpression( node ) { if ( node.callee.name === 'Date' && - node.arguments.length === 0 + node.arguments.length >= 0 && + node.arguments.length <= 2 ) { report( node ); } @@ -62,7 +63,9 @@ module.exports = { if ( node.callee.object && node.callee.object.name === 'Date' && - node.callee.property.name === 'now' + node.callee.property.name === 'now' && + node.arguments.length >= 0 && + node.arguments.length <= 2 ) { // Don't show an error if Date.now() is used within another Date expression. if ( diff --git a/packages/eslint-plugin/rules/no-direct-date.test.js b/packages/eslint-plugin/rules/no-direct-date.test.js index 6494e49e940..aa1cf63241c 100644 --- a/packages/eslint-plugin/rules/no-direct-date.test.js +++ b/packages/eslint-plugin/rules/no-direct-date.test.js @@ -36,7 +36,7 @@ const ruleTester = new RuleTester( { ruleTester.run( 'no-direct-date', rule, { valid: [ 'const date = select( CORE_USER ).getReferenceDate();', - 'const createTime = new Date( Date.now() - DAY_IN_SECONDS * 1.5 * 1000 ).toISOString();', + 'const createTime = new Date( DAY_IN_SECONDS * 1.5 * 1000, 20, 3 ).toISOString();', ], invalid: [ { @@ -57,5 +57,41 @@ ruleTester.run( 'no-direct-date', rule, { }, ], }, + { + code: 'const date = new Date( 12345 );', + errors: [ + { + message: + "Avoid using 'new Date()'. Use select( CORE_USER ).getReferenceDate() or add a comment explaining why the reference date should not be used here.", + }, + ], + }, + { + code: 'const timestamp = Date.now( 12345 );', + errors: [ + { + message: + "Avoid using 'Date.now()'. Use select( CORE_USER ).getReferenceDate() or add a comment explaining why the reference date should not be used here.", + }, + ], + }, + { + code: 'const date = new Date( 12345, 2 );', + errors: [ + { + message: + "Avoid using 'new Date()'. Use select( CORE_USER ).getReferenceDate() or add a comment explaining why the reference date should not be used here.", + }, + ], + }, + { + code: 'const timestamp = Date.now( 12345, 2 );', + errors: [ + { + message: + "Avoid using 'Date.now()'. Use select( CORE_USER ).getReferenceDate() or add a comment explaining why the reference date should not be used here.", + }, + ], + }, ], } ); From c7de71d253f7b502e24db9950dbd33cc7854c299 Mon Sep 17 00:00:00 2001 From: "Matthew Riley MacPherson (tofumatt)" Date: Fri, 4 Oct 2024 17:31:12 +0100 Subject: [PATCH 2/3] Remove redundant minimum argument check. --- packages/eslint-plugin/rules/no-direct-date.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/eslint-plugin/rules/no-direct-date.js b/packages/eslint-plugin/rules/no-direct-date.js index 5defcecef87..b9451154412 100644 --- a/packages/eslint-plugin/rules/no-direct-date.js +++ b/packages/eslint-plugin/rules/no-direct-date.js @@ -53,7 +53,6 @@ module.exports = { NewExpression( node ) { if ( node.callee.name === 'Date' && - node.arguments.length >= 0 && node.arguments.length <= 2 ) { report( node ); @@ -64,7 +63,6 @@ module.exports = { node.callee.object && node.callee.object.name === 'Date' && node.callee.property.name === 'now' && - node.arguments.length >= 0 && node.arguments.length <= 2 ) { // Don't show an error if Date.now() is used within another Date expression. From 34510c2b2302832e37f3d9aab880c885af0f9539 Mon Sep 17 00:00:00 2001 From: "Matthew Riley MacPherson (tofumatt)" Date: Wed, 9 Oct 2024 17:09:34 +0100 Subject: [PATCH 3/3] Update based on code review feedback. --- assets/js/util/convert-time.js | 5 ++++- packages/eslint-plugin/rules/no-direct-date.js | 3 +-- packages/eslint-plugin/rules/no-direct-date.test.js | 2 +- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/assets/js/util/convert-time.js b/assets/js/util/convert-time.js index 049d7812289..1885f142d8c 100644 --- a/assets/js/util/convert-time.js +++ b/assets/js/util/convert-time.js @@ -58,7 +58,10 @@ export const convertSecondsToArray = ( seconds ) => { export const convertDateStringToUNIXTimestamp = ( dateStringValue ) => { const unixTimestamp = dateStringValue && ! Number.isInteger( dateStringValue ) - ? // Valid use of `new Date()` with an argument. + ? // Valid use of `new Date()` with an argument, because this should only + // be passed full time strings, not `YYYY-MM-DD` style dates. + // + // See: https://github.com/google/site-kit-wp/pull/9459#discussion_r1790660073 // eslint-disable-next-line sitekit/no-direct-date new Date( dateStringValue ).getTime() : dateStringValue; diff --git a/packages/eslint-plugin/rules/no-direct-date.js b/packages/eslint-plugin/rules/no-direct-date.js index b9451154412..764971a01a7 100644 --- a/packages/eslint-plugin/rules/no-direct-date.js +++ b/packages/eslint-plugin/rules/no-direct-date.js @@ -62,8 +62,7 @@ module.exports = { if ( node.callee.object && node.callee.object.name === 'Date' && - node.callee.property.name === 'now' && - node.arguments.length <= 2 + node.callee.property.name === 'now' ) { // Don't show an error if Date.now() is used within another Date expression. if ( diff --git a/packages/eslint-plugin/rules/no-direct-date.test.js b/packages/eslint-plugin/rules/no-direct-date.test.js index aa1cf63241c..370a141a4ca 100644 --- a/packages/eslint-plugin/rules/no-direct-date.test.js +++ b/packages/eslint-plugin/rules/no-direct-date.test.js @@ -36,7 +36,7 @@ const ruleTester = new RuleTester( { ruleTester.run( 'no-direct-date', rule, { valid: [ 'const date = select( CORE_USER ).getReferenceDate();', - 'const createTime = new Date( DAY_IN_SECONDS * 1.5 * 1000, 20, 3 ).toISOString();', + 'const createTime = new Date( 2024, 5, 9 ).toISOString();', ], invalid: [ {