Skip to content

feat(moment-date-adapter): implement strict mode #16462

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.

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

Merged
merged 6 commits into from
Aug 6, 2019

Conversation

MatthiasKunnen
Copy link
Contributor

@MatthiasKunnen MatthiasKunnen commented Jul 5, 2019

Moment supports a strict flag which solves a lot of parsing errors. This PR allows setting this flag.

Why
Despite specifying a format, moment is very forgiving in the date strings it perceives as valid. Examples:

Input Format Forgiving Strict
1/2/2017 M/D/YYYY 2017-01-02 2017-01-02
1/2/2017 MM/DD/YYYY 2017-01-02 Invalid date
2/1/2017 MM-DD-YYYY 2017-02-01 Invalid date
Januar 1, 2017 MMMM D, YYYY 2017-01-01 Invalid date
Jan 1, 2017 MMMM D, YYYY 2017-01-01 Invalid date
2017-1-1 MMMM D, YYYY 2017-01-20 Invalid date
2017/12/24 MMMM D, YYYY 2017-01-20 Invalid date
2017/12/10 MMM D, YYYY 2017-01-20 Invalid date

Note
The moment docs state

In a later release, the parser will default to using strict mode.

This implementation follows the flag's default value in Moment(strict off). When Moment changes their default, the moment-date-adapter will follow.

@MatthiasKunnen MatthiasKunnen requested a review from mmalerba as a code owner July 5, 2019 15:22
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jul 5, 2019
@MatthiasKunnen MatthiasKunnen force-pushed the moment-adapter-strict-mode branch from f3d9f9f to 6d828cf Compare July 5, 2019 15:27
Moment supports a strict flag which solves a lot of parsing errors.

Examples:

| Input           | Format       | Forgiving  | Strict       |
|-----------------|--------------|------------|--------------|
| 1/2/2017        | M/D/YYYY     | 2017-01-02 | 2017-01-02   |
| 1/2/2017        | MM/DD/YYYY   | 2017-01-02 | Invalid date |
| Januari 1, 2017 | MMMM D, YYYY | 2017-01-01 | Invalid date |
| Jan 1, 2017     | MMMM D, YYYY | 2017-01-01 | Invalid date |
| 2017-1-1        | MMMM D, YYYY | 2017-01-20 | Invalid date |
@MatthiasKunnen MatthiasKunnen force-pushed the moment-adapter-strict-mode branch from 6d828cf to 8df3128 Compare July 5, 2019 15:35
format?: MomentFormatSpecification,
locale?: string,
): Moment {
const strict = this._options ? this._options.strict : undefined;
Copy link
Member

Choose a reason for hiding this comment

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

It looks like the this._options is used in a few places. Consider moving into a variable since it should minify a little better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've attempted to do so. Let me know if it needs further change since I'm not sure this is what you requested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@crisbeto, is the refactor as you requested?

@MatthiasKunnen MatthiasKunnen force-pushed the moment-adapter-strict-mode branch from 03cc7a9 to 4bd72de Compare July 7, 2019 16:53
@mmalerba mmalerba added feature This issue represents a new feature or feature request rather than a bug or bug fix target: minor This PR is targeted for the next minor release labels Jul 25, 2019
@mmalerba mmalerba added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Jul 26, 2019
@MatthiasKunnen
Copy link
Contributor Author

Anything holding up this PR @mmalerba?

@mmalerba
Copy link
Contributor

mmalerba commented Aug 2, 2019

We run all of the PRs against Google's codebase internally to make sure that they don't break any apps or tests. This one is waiting to be run

@mmalerba mmalerba merged commit dd42956 into angular:master Aug 6, 2019
@brgrz
Copy link

brgrz commented Aug 8, 2019

When will this be released as a new version, literally we need it right now :)

@mmalerba
Copy link
Contributor

mmalerba commented Aug 8, 2019

This will be included in the next minor release. These generally happen ~1/month

@brgrz
Copy link

brgrz commented Aug 20, 2019

@mmalerba I've just updated to 8.1.3 that was release 6 days ago and I don't see the strict option in it. Am I missing something?

Update: I can see here it wasn't included https://github.com/angular/components/releases/tag/8.1.3 Why would that be?

Update: Actually under commits I can see it was included but when I download 8.1.3 from npm the updated code is not there..only useUtc option exists.

@MatthiasKunnen
Copy link
Contributor Author

@brgrz, that is due to semantic versioning. 8.1.3 is a patch release. As this pr adds a new feature, it will arrive in the next minor release. See also the label, target: minor. Strict mode will arrive in 8.2.0.

You can read more about Angular's versioning here: https://angular.io/guide/releases

@MatthiasKunnen
Copy link
Contributor Author

@brgrz, 8.2.0 has been released. You can now use strict mode.

@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement feature This issue represents a new feature or feature request rather than a bug or bug fix target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants