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.

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

[moment_v2.x.x] Support moment v2.25 strict string parsing feature #3832

Merged

Conversation

valscion
Copy link
Contributor

@valscion valscion commented May 12, 2020

Other notes:

In moment v2.25, constructing a Moment object via moment(someString, true) got allowed.

Note that this PR intentionally does not fork the moment type declarations on the moment_v2.25.x version boundary. Forking would make updating these type declarations a pain in the future.

Hopefully the small comments I left in the type declarations will help users that aren't running moment v2.25 yet to understand that the new API would not work for them.

The previous locations for $ExpectError in these tests hid too many
errors and more modern Flow versions no longer accepted that.
@@ -169,6 +175,12 @@ declare class moment$Moment {
| void
): moment$Moment;
static utc(string: string, format: string | Array<string>): moment$Moment;
static utc(
string: string,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was a bit odd to see that moment() constructor made the first string argument be optional with ?string but moment.utc didn't.

Seems like #2875 did not do the changes for moment.utc constructor. If this is something that we'd want to have in flow-typed, I can do that in another PR — but it is not in the scope of this PR, in my opinion.

Copy link
Contributor

@AndrewSouthpaw AndrewSouthpaw left a comment

Choose a reason for hiding this comment

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

Seems reasonable. Calling this good.

@AndrewSouthpaw AndrewSouthpaw merged commit e7ad246 into flow-typed:master Jun 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants