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

Update date ESLint rule to flag usage of new Date() with arguments. #9459

Merged
merged 3 commits into from
Oct 9, 2024
Merged

Update date ESLint rule to flag usage of new Date() with arguments. #9459

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
10be599
Update date ESLint rule to flag usage of `new Date()` with arguments.
tofumatt Oct 3, 2024
c7de71d
Remove redundant minimum argument check.
tofumatt Oct 4, 2024
34510c2
Update based on code review feedback.
tofumatt Oct 9, 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
11 changes: 11 additions & 0 deletions .eslintrc.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,17 @@
"off"
]
}
},
{
"files": [
"webpack/*",
"**/__factories__/*",
"*.stories.js",
"*.test.js"
],
"rules": {
"sitekit/no-direct-date": "off"
aaemnnosttv marked this conversation as resolved.
Show resolved Hide resolved
}
}
],
"plugins": [
Expand Down
4 changes: 4 additions & 0 deletions assets/js/components/GoogleChart/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 ) );
} );

Expand All @@ -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(
Expand Down
2 changes: 2 additions & 0 deletions assets/js/components/notifications/BannerNotification/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
5 changes: 5 additions & 0 deletions assets/js/modules/adsense/util/data-mock.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
2 changes: 2 additions & 0 deletions assets/js/modules/adsense/util/site-stats-data.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 2 additions & 0 deletions ...s/modules/analytics-4/components/dashboard/DashboardAllTrafficWidgetGA4/UserCountGraph.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: __(
Expand Down
2 changes: 2 additions & 0 deletions assets/js/modules/analytics-4/datastore/custom-dimensions-gathering-data.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }` ],
Expand Down
2 changes: 2 additions & 0 deletions assets/js/modules/analytics-4/datastore/partial-data.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down
2 changes: 2 additions & 0 deletions assets/js/modules/search-console/components/common/AnalyticsStats.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
5 changes: 5 additions & 0 deletions assets/js/modules/search-console/util/data-mock.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
4 changes: 3 additions & 1 deletion assets/js/util/convert-time.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is valid if it uses a timestamp as shown in the docblock (complete with timezone), however this would be incorrect for one of our YYYY-MM-DD date strings as noted in the description of the issue.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, right, I missed the ternary showing that this would be a string in this case 😓 Thanks 👍🏻

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, actually, this is only used in one place and is designed to accept strings:

site-kit-wp/assets/js/modules/analytics-4/datastore/base.js

Lines 78 to 82 in c7de71d

baseModuleStore.actions.setPropertyCreateTime = ( value ) => {
return originalSetPropertyCreateTime(
convertDateStringToUNIXTimestamp( value )
);
};

Linting isn't the right way to protect against invalid arguments here, it would need to be an invariant on the length of the datetime string.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed, SGTM

: dateStringValue;

if ( isNaN( unixTimestamp ) || ! unixTimestamp ) {
Expand Down
6 changes: 6 additions & 0 deletions assets/js/util/dates.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 );
}

Expand Down Expand Up @@ -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 );
}
12 changes: 1 addition & 11 deletions packages/eslint-plugin/configs/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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' ],
},
};
5 changes: 3 additions & 2 deletions packages/eslint-plugin/rules/no-direct-date.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ module.exports = {
NewExpression( node ) {
if (
node.callee.name === 'Date' &&
node.arguments.length === 0
node.arguments.length <= 2
Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer using the simpler logical operator where possible. WDYT?

Suggested change
node.arguments.length <= 2
node.arguments.length < 3

Copy link
Collaborator Author

@tofumatt tofumatt Oct 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I usually find the "less than or equal to" easier to scan, because I don't have to do complex arithmetic in my head.

With < 3 I need to calculate something eg. "What comes before 3 here, logically? And should it be an Integer or a Floating point number?" etc., but for <= 2 I can just go "oh, it's two or less". 😅

Copy link
Collaborator

Choose a reason for hiding this comment

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

but for <= 2 I can just go "oh, it's two or less".

I don't feel super strongly about it, so happy to keep what you have. Removing the "or" from that thought is what I mean by logically simpler.

) {
report( node );
}
Expand All @@ -62,7 +62,8 @@ module.exports = {
if (
node.callee.object &&
node.callee.object.name === 'Date' &&
node.callee.property.name === 'now'
node.callee.property.name === 'now' &&
node.arguments.length <= 2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Date.now doesn't accept any parameters, so this should be triggered regardless of being used properly or not :)

) {
// Don't show an error if Date.now() is used within another Date expression.
if (
Expand Down
38 changes: 37 additions & 1 deletion packages/eslint-plugin/rules/no-direct-date.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();',
Copy link
Collaborator

Choose a reason for hiding this comment

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

This instance isn't exactly using the constructor properly. The arguments are year, monthIndex, day which don't quite align with the usage here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, true, I was just using bogus values but I'll have them reflect real-world usage 👍🏻

],
invalid: [
{
Expand All @@ -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.",
},
],
},
],
} );
Loading