fix(facade): cache original date format string

[awerlang]

Please check if the PR fulfills these requirements

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

What kind of change does this PR introduce? (check one with "x")

[X] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior? (You can also link to an open issue here)

There's a useless cache being maintained in the Intl facade for dates. Its purpose it's to avoid a possibly expensive RegExp operation. Unfortunately the expensive result was being cached in the wrong entry (null).

What is the new behavior?

Add to the correct cache entry.

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[X] No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:

[pkozlowski-opensource]

@awerlang this looks good to me, generally speaking, well spotted. What makes me nervous, though, is that we don't seem to have any tests that would help us to catch this / similar issues in the future. Could you think of the way of testing this (ex. formatting with a given format and then trying to format with null one to get an unexpected format from the cache?).

[vicb]

May be const cacheKey = format TS will complain if the key gets modified ?

[awerlang]

Could you think of the way of testing this (ex. formatting with a given format and then trying to format with null one to get an unexpected format from the cache?).

Will do.

[awerlang]

I decided to go with a spy. It's the only way we can detect the presence of a transparent cache.

[vicb]

use format

[awerlang]

done

[vicb]

I'm not sure if this test is valuable ?

• It exposes impl details,
• It doesn't really test the cache behavior.

@pkozlowski-opensource ?

[awerlang]

I see your point. I can export a new DateFormatCache class instead of Map and use exclusively for this caching. WDYT?

[vicb]

I wouldn't mind if we remove this test

[awerlang]

Removed the test as requested. Let me know if there's anything blocking.

[vicb]

Thanks for the update. LGTM.

[angular-automatic-lock-bot]

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.}