Skip to content

Commit

Permalink
feat(common): throw error for suspicious date patterns (#59798)
Browse files Browse the repository at this point in the history
Adjusts the date pipe and formatDate function to detect suspicious usages of the week-numbering year formatter without including the week number, as this is often confused for the calendar year and likely to result in incorrect results near New Years, meaning that those issues aren't typically caught during development. This commit starts throwing a development-only error to reveal this issue right away.

BREAKING CHANGE:

Using the `Y` formatter (week-numbering year) without also including `w` (week number) is now detected as suspicious date pattern, as `y` is typically intended.

PR Close #59798
  • Loading branch information
shicks authored and kirjs committed Feb 24, 2025
1 parent e4ef74d commit 74cceba
Show file tree
Hide file tree
Showing 4 changed files with 65 additions and 13 deletions.
2 changes: 2 additions & 0 deletions goldens/public-api/common/errors.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ export const enum RuntimeErrorCode {
// (undocumented)
REQUIRED_INPUT_MISSING = 2954,
// (undocumented)
SUSPICIOUS_DATE_FORMAT = 2300,
// (undocumented)
TOO_MANY_PRELOADED_IMAGES = 2961,
// (undocumented)
TOO_MANY_PRIORITY_ATTRIBUTES = 2966,
Expand Down
3 changes: 3 additions & 0 deletions packages/common/src/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ export const enum RuntimeErrorCode {
// NgForOf errors
NG_FOR_MISSING_DIFFER = -2200,

// I18n errors
SUSPICIOUS_DATE_FORMAT = 2300,

// Keep 2800 - 2900 for Http Errors.

// Image directive errors
Expand Down
33 changes: 33 additions & 0 deletions packages/common/src/i18n/format_date.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,11 @@
* found in the LICENSE file at https://angular.dev/license
*/

import {
ɵRuntimeError as RuntimeError,
ɵformatRuntimeError as formatRuntimeError,
} from '@angular/core';

import {
FormatWidth,
FormStyle,
Expand All @@ -24,6 +29,7 @@ import {
Time,
TranslationWidth,
} from './locale_data_api';
import {RuntimeErrorCode} from '../errors';

export const ISO8601_DATE_REGEX =
/^(\d{4,})-?(\d\d)-?(\d\d)(?:T(\d\d)(?::?(\d\d)(?::?(\d\d)(?:\.(\d+))?)?)?(Z|([+-])(\d\d):?(\d\d))?)?$/;
Expand Down Expand Up @@ -104,6 +110,10 @@ export function formatDate(
}
}

if (typeof ngDevMode === 'undefined' || ngDevMode) {
assertValidDateFormat(parts);
}

let dateTimezoneOffset = date.getTimezoneOffset();
if (timezone) {
dateTimezoneOffset = timezoneToOffset(timezone, dateTimezoneOffset);
Expand All @@ -123,6 +133,29 @@ export function formatDate(
return text;
}

/**
* Asserts that the given date format is free from common mistakes. Throws an
* error if one is found (except for the case of all "Y", in which case we just
* log a warning). This should only be called in development mode.
*/
function assertValidDateFormat(parts: string[]) {
if (parts.some((part) => /^Y+$/.test(part)) && !parts.some((part) => /^w+$/.test(part))) {
// "Y" indicates "week-based year", which differs from the actual calendar
// year for a few days around Jan 1 most years. Unless "w" is also
// present (e.g. a date like "2024-W52") this is likely a mistake. Users
// probably meant "y" instead.
const message = `Suspicious use of week-based year "Y" in date pattern "${parts.join(
'',
)}". Did you mean to use calendar year "y" instead?`;
if (parts.length === 1) {
// NOTE: allow "YYYY" with just a warning, since it's used in tests.
console.error(formatRuntimeError(RuntimeErrorCode.SUSPICIOUS_DATE_FORMAT, message));
} else {
throw new RuntimeError(RuntimeErrorCode.SUSPICIOUS_DATE_FORMAT, message);
}
}
}

/**
* Create a new Date object with the given date value, and the time set to midnight.
*
Expand Down
40 changes: 27 additions & 13 deletions packages/common/test/i18n/format_date_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,14 @@ describe('Format date', () => {
BBBBB: 'at night',
};

// Suppress console warnings for 'YYYY' patterns.
const consoleError = console.error;
spyOn(console, 'error').and.callFake((...args: unknown[]) => {
if (!/Suspicious use of week-based year/.test(String(args))) {
consoleError(...args);
}
});

Object.keys(dateFixtures).forEach((pattern: string) => {
expectDateFormatAs(date, pattern, dateFixtures[pattern]);
});
Expand Down Expand Up @@ -450,17 +458,23 @@ describe('Format date', () => {

// https://github.com/angular/angular/issues/38739
it('should return correct ISO 8601 week-numbering year for dates close to year end/beginning', () => {
expect(formatDate('2013-12-27', 'YYYY', 'en')).toEqual('2013');
expect(formatDate('2013-12-29', 'YYYY', 'en')).toEqual('2013');
expect(formatDate('2013-12-31', 'YYYY', 'en')).toEqual('2014');
expect(formatDate('2013-12-27', `YYYY 'W'ww`, 'en')).toEqual('2013 W52');
expect(formatDate('2013-12-29', `YYYY 'W'ww`, 'en')).toEqual('2013 W52');
expect(formatDate('2013-12-31', `YYYY 'W'ww`, 'en')).toEqual('2014 W01');

// Dec. 31st is a Sunday, last day of the last week of 2023
expect(formatDate('2023-12-31', 'YYYY', 'en')).toEqual('2023');
expect(formatDate('2023-12-31', `YYYY 'W'ww`, 'en')).toEqual('2023 W52');

expect(formatDate('2010-01-02', 'YYYY', 'en')).toEqual('2009');
expect(formatDate('2010-01-04', 'YYYY', 'en')).toEqual('2010');
expect(formatDate('0049-01-01', 'YYYY', 'en')).toEqual('0048');
expect(formatDate('0049-01-04', 'YYYY', 'en')).toEqual('0049');
expect(formatDate('2010-01-02', `YYYY 'W'ww`, 'en')).toEqual('2009 W53');
expect(formatDate('2010-01-04', `YYYY 'W'ww`, 'en')).toEqual('2010 W01');
expect(formatDate('0049-01-01', `YYYY 'W'ww`, 'en')).toEqual('0048 W53');
expect(formatDate('0049-01-04', `YYYY 'W'ww`, 'en')).toEqual('0049 W01');
});

it('should throw an error when using YYYY incorrectly', () => {
expect(() => formatDate('2013-12-31', `YYYY/MM/dd`, ɵDEFAULT_LOCALE_ID)).toThrowError(
/.*Suspicious use of week-based year "Y".*/,
);
});

// https://github.com/angular/angular/issues/53813
Expand All @@ -480,11 +494,11 @@ describe('Format date', () => {

// https://github.com/angular/angular/issues/40377
it('should format date with year between 0 and 99 correctly', () => {
expect(formatDate('0098-01-11', 'YYYY', ɵDEFAULT_LOCALE_ID)).toEqual('0098');
expect(formatDate('0099-01-11', 'YYYY', ɵDEFAULT_LOCALE_ID)).toEqual('0099');
expect(formatDate('0100-01-11', 'YYYY', ɵDEFAULT_LOCALE_ID)).toEqual('0100');
expect(formatDate('0001-01-11', 'YYYY', ɵDEFAULT_LOCALE_ID)).toEqual('0001');
expect(formatDate('0000-01-11', 'YYYY', ɵDEFAULT_LOCALE_ID)).toEqual('0000');
expect(formatDate('0098-01-11', `YYYY 'W'ww`, ɵDEFAULT_LOCALE_ID)).toEqual('0098 W02');
expect(formatDate('0099-01-11', `YYYY 'W'ww`, ɵDEFAULT_LOCALE_ID)).toEqual('0099 W02');
expect(formatDate('0100-01-11', `YYYY 'W'ww`, ɵDEFAULT_LOCALE_ID)).toEqual('0100 W02');
expect(formatDate('0001-01-11', `YYYY 'W'ww`, ɵDEFAULT_LOCALE_ID)).toEqual('0001 W02');
expect(formatDate('0000-01-11', `YYYY 'W'ww`, ɵDEFAULT_LOCALE_ID)).toEqual('0000 W02');
});

// https://github.com/angular/angular/issues/26922
Expand Down

0 comments on commit 74cceba

Please sign in to comment.