feat(common): throw error for suspicious date patterns #59798

shicks · 2025-01-29T23:39:23Z

The banned patterns are as folllows:

Y (week-based year) without w (week of year)
D (day of year) in the same pattern as M (month)

Such patterns are extremely likely to indicate an error. In particular, week-based year renders an incorrect year about 1% of the time (specifically, a few days before or after Jan 1 each year). Moreover, since it's correct the vast majority of the time, these errors are very difficult to notice and debug.

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/main/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.dev application / infrastructure changes
Other... Please describe: Adds additional error-checking (at development time only) to avoid error-prone usage

What is the current behavior?

formatDate(new Date('2024-12-31'), 'YYYY')) // returns 2025

Issue Number: N/A

What is the new behavior?

This is now an error in ngDevMode, directing the user to use y instead of Y.

Does this PR introduce a breaking change?

Yes
No
Only in ngDevMode

Other information

JeanMeche · 2025-01-30T01:53:59Z

While I get where you're going with this, I think the idea goes in the right direction, every end of the year we get 5-10 reports of people using YYYY instead f yyyy.
I expect this to be way too breaking. (It would be interesting to know what a TGP says about this)

Wouldn'it be more reasonable to log a warning/error ?

Edit: Phrasing, this is not a random community PR 😅

packages/common/src/i18n/format_date.ts

shicks · 2025-01-30T02:13:41Z

While I get where you're going with this, I think the idea goes in the right direction, every end of the year we get 5-10 reports of people using YYYY instead f yyyy. I expect this to be way too breaking. (It would be interesting to know what a TGP says about this)

Wouldn'it be more reasonable to log a warning/error ?

Edit: Phrasing, this is not a random community PR 😅

I did a combined TGP with both this error and an analogous one in goog.i18n.DateTimeFormat and fixed ~500 files between them both - the vast majority were Angular projects, though many of those were using a Closure adapter and weren't actually exercising this particular function. At this point, short of backsliding (which appears to be maybe one or two per day), the depot is clean w.r.t. this error. Externally, I do expect this to be somewhat disruptive, but I would hope nobody is serving production with n 8000 gDevMode on, so this should be a no-op in production.

EDIT: See go/lsc-date-format-templates.

JeanMeche · 2025-01-30T02:36:34Z

packages/common/src/i18n/format_date.ts

@@ -83,6 +83,27 @@ export function formatDate(
  locale: string,
  timezone?: string,
 ): string {
+  if (ngDevMode) {
+    if (format.includes('Y') && !format.includes('w')) {


Wdyt of excluding "YYYY" from the check ?

I don't love it: I saw plenty of those in the depot and they were all wrong.

That said, I'm working on getting the tests to pass, and it's a bit awkward. Not sure what the best approach would be there.

shicks · 2025-01-30T03:00:41Z

I've pushed an update the fixes the tests that this broke, but I'm not happy about it. I'd rather not back off on YYYY by itself because in my experience it's still very likely to be wrong, but to avoid tripping over this in testing, it means we need to always add the week as well. It's not unreasonable for some of the tests, but the ones that are just a big object literal are a lot uglier now (I could make them string keys instead of using _ but it's not clear either of these are good solutions).

The equivalent errorprone check in Java has a @SuppressWarnings-based opt-out, but I don't know of a great way to do that here.

JeanMeche · 2025-01-30T03:06:48Z

One idea could be that for YYYY specifically we log a warning but we still throw an error in all the other cases ?

shicks · 2025-01-30T03:09:20Z

One idea could be that for YYYY specifically we log a warning but we still throw an error in all the other cases ?

That seems like a reasonable compromise. Is there a standard way to test logged warnings/thrown errors?

JeanMeche · 2025-01-30T03:16:32Z

When we're in a DI context, we use the Console token.

angular/packages/platform-browser/src/hydration.ts

Lines 159 to 165 in 9723f03

    
           const console = inject(Console); 
        
           const message = formatRuntimeError( 
        
             RuntimeErrorCode.UNSUPPORTED_ZONEJS_INSTANCE, 
        
             'Angular detected that hydration was enabled for an application ' + 
        
               'that uses a custom or a noop Zone.js implementation. ' + 
        
               'This is not yet a fully supported configuration.', 
        
           );

This is not the case with formatDate which is a standalone function. In those cases we usually use a spy.

angular/packages/common/test/directives/ng_optimized_image_spec.ts

Lines 1649 to 1655 in 9723f03

    
           const consoleWarnSpy = spyOn(console, 'warn'); 
        
           fixture.detectChanges(); 
        
           expect(consoleWarnSpy.calls.count()).toBe(1); 
        
           expect(consoleWarnSpy.calls.argsFor(0)[0]).toMatch( 
        
             /your images may be hosted on the Imgix CDN/, 
        
           );

With warning being logged using formatRuntimeError.

shicks · 2025-01-31T03:16:12Z

PTAL

AndrewKushnir

@shicks thanks a lot for the changes and helping to update Google's codebase (to make it compatible with this change) 👍

Just left a comment with a couple minor proposals.

packages/common/src/i18n/format_date.ts

JoostK · 2025-02-12T13:16:59Z

This doesn't currently account for occurrences within escaped strings within the pattern, e.g.

'MMMM \'in the Year\' yyyy'

would incorrectly be rejected. It seems safer to perform this check internally within formatDate based on the parts that are being parsed out.

I'd also consider this a feature, not a fix. It is potentially a hard breakage for occurrences within libraries.

packages/common/src/i18n/format_date.ts

shicks · 2025-02-13T21:55:39Z

@JoostK

This doesn't currently account for occurrences within escaped strings within the pattern, e.g.
'MMMM \'in the Year\' yyyy'
would incorrectly be rejected. It seems safer to perform this check internally within formatDate based on the parts that are being parsed out.

FWIW, in all of Google's codebase, I did not run into any cases where this was an issue. That's not to say it's impossible, but I suspect it's more theoretical than actual.

That said, I think the easiest way to handle this correctly would be to pass an additional mutable argument into the DateFormatter function to keep track of the two relevant conditions ('Y' and 'w'), and then move the check to be before returning the text. E.g. shicks@069e82e. Or alternatively shicks@66022da just iterates over the split strings. WDYT, would we prefer either of these options?

I'd also consider this a feature, not a fix. It is potentially a hard breakage for occurrences within libraries.

I'm talking with @AndrewKushnir about waiting for v20 to release this. If you want a concrete change to the commit message, please say what it should be instead.

I've also removed the D check since I realized this formatter doesn't actually support that character.

EDIT: added a second alternative, fixed to "reply" to the correct reviewer.

AndrewKushnir · 2025-02-13T22:22:44Z

That said, I think the easiest way to handle this correctly would be to pass an additional mutable argument into the DateFormatter function to keep track of the two relevant conditions ('Y' and 'w'), and then move the check to be before returning the text. E.g. shicks@069e82e - WDYT, would we prefer this option?

cc @JoostK

JoostK · 2025-02-13T22:30:45Z

@kirjs

This doesn't currently account for occurrences within escaped strings within the pattern, e.g.
'MMMM 'in the Year' yyyy'
would incorrectly be rejected. It seems safer to perform this check internally within formatDate based on the parts that are being parsed out.

FWIW, in all of Google's codebase, I did not run into any cases where this was an issue. That's not to say it's impossible, but I suspect it's more theoretical than actual.

That said, I think the easiest way to handle this correctly would be to pass an additional mutable argument into the DateFormatter function to keep track of the two relevant conditions ('Y' and 'w'), and then move the check to be before returning the text. E.g. shicks@069e82e - WDYT, would we prefer this option?

This is very similar to what I had in mind, this feels much more robust 👍

I'd also consider this a feature, not a fix. It is potentially a hard breakage for occurrences within libraries.

I'm talking with @AndrewKushnir about waiting for v20 to release this. If you want a concrete change to the commit message, please say what it should be instead.

It was mostly concerning the choice for fix without breaking change note that made it look this may end up in v19, as fixes are typically merged to all active branches.

I'd go with something like this:

feat(common): throw error for suspicious date patterns

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 YYYY formatter (week-numbering year) without also including dd (week number) is now detected as suspicious date pattern, as yyyy is typically intended.

One area where the note can probably be improved is to mention how this error can be resolved. I don't think it's necessary to mention that it's not possible to disable this error in intentional cases, given that we really don't expect this to be intentional.

I'm on mobile at the moment so haven't cross-refenced whether I actually mention the correct date patterns in the above suggestion.

shicks · 2025-02-13T22:36:56Z

I actually really like the second alternative I added above (shicks@66022da), since it's a pretty minor change and only has any effect at all in ngDevMode.

I was also thinking a bit more about suppressing the warning. The fact that "YYYY" by itself is o 6D40 nly a console warning because it's used in tests is really smelly, but the problem is that there's not a reasonable way for the developer to say "no, I really mean this". In the Java prior art (errorprone.info) it's suppressible with a @SuppressWarnings("MisusedWeekYear") annotation, which works great because it's compile-time, but we don't have any sort of equivalent here for this runtime check. But I wonder if it wouldn't be better to add a new format character ! that doesn't emit anything but suppresses validity checks for the whole format string? It would be breaking if anybody is using an unquoted ! in their format string, but that also seems pretty unlikely, and it would be more general in that if you really do have a reason to use it, you can use it however you want and you won't get a pesky console message, either.

shicks · 2025-02-13T22:58:44Z

I folded in the fix for the false positive on quoted chars.

I also put together a draft shicks@97d9405 for adding a new ! control character. I think the most relevant question to my mind is whether it's cleaner w.r.t. the tests. Specifically, it removes the need for special treatment of Y-only formats, and it ensures that any broken external users have a reasonable way out if this would impact them.

shicks · 2025-02-14T21:41:16Z

Fixed the lint

shicks · 2025-02-19T16:53:57Z

Ping @AndrewKushnir - is there anything left to do on this?

AndrewKushnir · 2025-02-19T20:55:24Z

Ping @AndrewKushnir - is there anything left to do on this?

Discussed offline: the next step is to run a TGP and if everything looks good, this PR will be ready for merge into the main branch, which is now open to breaking changes (in a context of v20 release).

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.

AndrewKushnir · 2025-02-21T17:41:12Z

Caretaker note (cc @kirjs): this PR contains a breaking change and we'll need one more TGP early next week (before the merge). Once the TGP results are available, please merge and sync this PR individually (without any other changes in a sync CL).

shicks · 2025-02-24T16:47:27Z

I ran a TGP overnight and found a single breakage, which is now fixed. I'll kick off another one right now to be ready with fix-forwards when this lands, but we should consider this ready-to-merge internally.

kirjs · 2025-02-24T21:31:52Z

Caretaker note: After discussing with @AndrewKushnir I'm going to switch target to minor and merge this to allow backsliding, as we're otherwise blocked on 60024.

kirjs · 2025-02-24T22:15:05Z

This PR was merged into the repository by commit 74cceba.

The changes were merged into the following branches: main

angular-automatic-lock-bot · 2025-04-02T00:12:12Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

pullapprove bot requested a review from pkozlowski-opensource January 29, 2025 23:39

JeanMeche reviewed Jan 30, 2025

View reviewed changes

packages/common/src/i18n/format_date.ts Outdated Show resolved Hide resolved

JeanMeche reviewed Jan 30, 2025

View reviewed changes

alxhub approved these changes Feb 12, 2025

View reviewed changes

pullapprove bot requested review from atscott and kirjs February 12, 2025 00:02

shicks force-pushed the patch-1 branch 2 times, most recently from 9e71f9e to fcfabb6 Compare February 12, 2025 00:52

angular-robot bot added the area: common Issues related to APIs in the @angular/common package label Feb 12, 2025

ngbot bot added this to the Backlog milestone Feb 12, 2025

AndrewKushnir approved these changes Feb 12, 2025

View reviewed changes

packages/common/src/i18n/format_date.ts Outdated Show resolved Hide resolved

shicks force-pushed the patch-1 branch from fcfabb6 to 6ec9a66 Compare February 12, 2025 04:46

kirjs removed their request for review February 12, 2025 14:39

pullapprove bot requested a review from kirjs February 12, 2025 14:39

kirjs approved these changes Feb 12, 2025

View reviewed changes

packages/common/src/i18n/format_date.ts Outdated Show resolved Hide resolved

packages/common/src/i18n/format_date.ts Outdated Show resolved Hide resolved

pullapprove bot requested a review from kirjs February 12, 2025 15:54

shicks force-pushed the patch-1 branch from 6ec9a66 to 764b66f Compare February 13, 2025 21:55

shicks force-pushed the patch-1 branch from 764b66f to b50a11b Compare February 13, 2025 22:42

AndrewKushnir added the target: major This PR is targeted for the next major release label Feb 14, 2025

AndrewKushnir changed the title ~~fix(common): ban two generally-incorrect date format patterns~~ feat(common): throw error for suspicious date patterns Feb 14, 2025

AndrewKushnir added breaking changes and removed detected: feature PR contains a feature commit detected: breaking change PR contains a commit with a breaking change labels Feb 14, 2025

shicks force-pushed the patch-1 branch from 84dbc1f to f48f454 Compare February 14, 2025 21:39

angular-robot bot added detected: breaking change PR contains a commit with a breaking change detected: feature PR contains a feature commit labels Feb 14, 2025

shicks force-pushed the patch-1 branch from f48f454 to 520cf07 Compare February 19, 2025 16:56

AndrewKushnir added the action: global presubmit The PR is in need of a google3 global presubmit label Feb 19, 2025

shicks force-pushed the patch-1 branch from 520cf07 to 7140bd1 Compare February 21, 2025 00:30

AndrewKushnir added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Feb 21, 2025

AndrewKushnir added action: merge The PR is ready for merge by the caretaker and removed action: global presubmit The PR is in need of a google3 global presubmit labels Feb 24, 2025

kirjs added target: minor This PR is targeted for the next minor release target: major This PR is targeted for the next major release and removed target: major This PR is targeted for the next major release target: minor This PR is targeted for the next minor release labels Feb 24, 2025

kirjs closed this in 74cceba Feb 24, 2025

angular-automatic-lock-bot bot locked and limited conversation to collaborators Apr 2, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(common): throw error for suspicious date patterns #59798

feat(common): throw error for suspicious date patterns #59798

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

feat(common): throw error for suspicious date patterns #59798

feat(common): throw error for suspicious date patterns #59798

Uh oh!

Conversation

Uh oh!

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!