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

[feature] Prevent toISOString converting to UTC (issue #1751) #4341

Merged
merged 4 commits into from Dec 17, 2017

Conversation

Projects
None yet
3 participants
@ashsearle
Contributor

ashsearle commented Dec 3, 2017

A minimal effort to allow users to opt-out of UTC conversion when calling toISOString():

moment().toISOString();
moment().toISOString({ utc: false });

Example while running in Europe/London timezone:

moment('2017-07-01').toISOString(); // 2017-06-30T23:00:00.000Z
moment('2017-07-01').toISOString({ utc: false }); // 2017-07-01T00:00:00.000+01:00

The PR preserves existing behaviour of converting to UTC by default.

@icambron

This comment has been minimized.

Member

icambron commented Dec 5, 2017

I love the option object pattern, but it's not how Moment works stylistically. Maybe just a boolean arg?

@ashsearle

This comment has been minimized.

Contributor

ashsearle commented Dec 5, 2017

Sure, I'm happy to change. Which way round do you want it: should toISOString() be the same as toISOString(true) or toISOString(false)?

I guess a single argument is also easy enough to expand in future too: e.g. by passing in 60 or +01:00 to get an ISO string in the specified offset...?

@icambron

This comment has been minimized.

Member

icambron commented Dec 5, 2017

Right, something like that. Honestly, the object is better, but when in Rome...

On defaults, I think it's more intuitive to default to false.

@ashsearle

This comment has been minimized.

Contributor

ashsearle commented Dec 5, 2017

Updated...

Is the function parameter name OK?

@icambron

This comment has been minimized.

Member

icambron commented Dec 5, 2017

The name seems fine

@marwahaha marwahaha removed the Needs docs label Dec 14, 2017

@marwahaha marwahaha merged commit 0c795e8 into moment:develop Dec 17, 2017

3 checks passed

Title Your title looks great!
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment