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

toDate returns a copy of the internal date object #3191

Closed
wants to merge 2 commits into
base: develop
from

Conversation

Projects
None yet
5 participants
@jonathathan
Contributor

jonathathan commented May 19, 2016

moment#toDate currently returns the internal date object, which can lead to strange behaviour if the moment object is then modified. See this fiddle for an example.

@icambron

This comment has been minimized.

Member

icambron commented May 19, 2016

+1

@maggiepint

This comment has been minimized.

Member

maggiepint commented May 19, 2016

Is there any reason why we can't just call

return new Date(this.valueOf());

and do away with the ternary? Seems to me it would have the same result, and it would work correctly with Tim's pull request that we've been talking about bringing in.

In either case, some kind of a unit test should be added for it. It seems like a trivial test case, but that way we ensure that nobody puts it back the other way for some strange reason.

@icambron

This comment has been minimized.

Member

icambron commented May 20, 2016

@maggiepint I agree on both points.

@jonathathan

This comment has been minimized.

Contributor

jonathathan commented May 23, 2016

@maggiepint @icambron addressed your suggestions, let me know what you think. Happy to take another pass at it.

@ichernev

This comment has been minimized.

Contributor

ichernev commented Jun 5, 2016

Looks great. Will merge in next release.

@ichernev ichernev closed this Jun 5, 2016

@ichernev ichernev reopened this Jun 5, 2016

@ichernev

This comment has been minimized.

Contributor

ichernev commented Jun 14, 2016

Merged in 1df38b4

@ichernev ichernev closed this Jun 14, 2016

ichernev added a commit that referenced this pull request Jun 14, 2016

Merge pull request #3191 from jonathathan:develop
toDate returns a copy of the internal date object

@mj1856 mj1856 added this to the 2.14.0 milestone Jul 5, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment