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

Be careful when falling back to Date constructor #1407

Closed
ichernev opened this issue Jan 9, 2014 · 136 comments
Closed

Be careful when falling back to Date constructor #1407

ichernev opened this issue Jan 9, 2014 · 136 comments
Labels

Comments

@ichernev
Copy link
Contributor

@ichernev ichernev commented Jan 9, 2014

If you clicked on the warning link and landed here:

moment construction using a non-iso string is deprecated. What this means is you can safely do

> moment("2014-04-25T01:32:21.196Z");  // iso string, utc timezone
> moment("2014-04-25T01:32:21.196+0600");  // iso string with timezone
> moment("2014 04 25", "YYYY MM DD"); // string with format

but you can't do reliably (therefore its being deprecated)

> moment("2014/04/25");
> moment("Thu Apr 24 2014 12:32:21 GMT-0700 (PDT)");
> moment("some random string that looks like date");

In the upcoming releases a few more well defined formats might be added, but the general idea of using any random string to construct a date is no more. If you really want that behavior just do moment(new Date("random string that contains date")), but moment won't be responsible for such convertions. Read below for the rationale about the change

As described in #1403 (comment), but also recent debugging c4d9f6e and 94d6c90 caused by https://travis-ci.org/moment/moment/builds/16672288 brings me to the conclusion that

using Date to create a moment from string, passed from the user is extremely unpredictable, and it turns out to work somewhat and then bite you in an edge case when you're not careful

So I think we need an option to disable it, for tests at the very least. I've debugged that numerous times thank to our old friend IE6 and friends.

You'd pass a string only, hoping that it would hit one of the predefined formats, but instead it hits Date and it sort-of works, sometimes. I think it should be clear to users when they're hitting this case, and that it might not work well on all browsers.

The only use-case for using Date to parse input is when a) it is entered by a web user (moment's user's user :)) and b) the web user can see how we're interpreting the date entered. Every other case is just silently waiting to break at some point.

@icambron
Copy link
Member

@icambron icambron commented Jan 9, 2014

+1. I totally agree. We should start issuing a deprecation warning right away. Some additional support:

  • That a string might go into the Date constructor or the ISO parser is ugly and confusing
  • The most reliable way of using the Date string constructor was always new Date('2014-01-01'), which is now handled by the ISO parser anyway (i.e. moment('2014-01-01') doesn't use the Date constructor anyway).
  • I'm pretty sure the constructor works differently in different locales for the same browser. E.g. new Date("05/06/2014") is May 6 in American browsers but June 5 in English browsers. So even if you test all your browsers, did you test all your locales? It's terrible for us to even pretend this stuff works.
  • If you really want to use the date constructor, you can always just use the date constructor and pass the date into Moment.
  • We could make moment(string) extensible, and then someone could create a plugin that either uses a list of well-known formats or just the Date constructor to parse them.
  • If we remove the Date constructor fall-back, we can actually make the ISO parser less strict, since it won't be preempting a better parse. So that's a win too.
@ichernev
Copy link
Contributor Author

@ichernev ichernev commented Jan 14, 2014

The idea I liked most is for extensible constructor -- even if we disable Date parsing in moment, we should make it easy for people to migrate.

So lets do this:

moment.createFromInputFallback = function(config) { config._d = new Date(config._i); };

And call this method in the else clause of makeDateFromInput. In 3.0 we'll just remove this function, or place it under another name, so that one can just assign to the preset.

For tests right now, we can put a different function that throws an exception if called. I hope that would work just fine.

Inside we can print using console.warn (would go to stderr in node), only the first time it is called (like global deprecation) -- this is to let users know what awaits them.

@st3fan
Copy link

@st3fan st3fan commented Apr 23, 2014

I don't understand this deprecation warning. I am using moment.js as documented, in an Angular.JS filter:

app.filter('scanDate', function() {
    return function(input) {
        var m = moment(input);
        return m.format("dddd, MMMM Do YYYY");
    };
});

Where input is an ISO formatted date string.

What do I need to do different to deal with this future deprecation?

If this deprecation warning is for the developers of this library and not for the users of it then I am going to suggest to remove it.

@icambron
Copy link
Member

@icambron icambron commented Apr 23, 2014

@st3fan Are you sure it's an ISO-formatted string? Moment is claiming that it isn't; it's saying that it can't parse it, and it is instead resorting to passing the string to new Date(s), which happens to work. That's the feature we're removing and are warning you about. In the future, it will fail. Your options are:

  1. Ensure that the input really is ISO-8601 compliant (and file a bug if you think it is but are getting the warning anyway)
  2. Parse the input explicitly using moment(input, string)

The warning is very much for you :)

@icambron
Copy link
Member

@icambron icambron commented Apr 23, 2014

This is implemented, so I'm closing it.

@icambron icambron closed this Apr 23, 2014
@Meekohi
Copy link

@Meekohi Meekohi commented Apr 25, 2014

If we deprecate everything in Javascript that doesn't work on a side-case there will be no features left. ;) Looking forward to at least a few other "sane" date formats being added back in. Explicitly creating a Date or including the format string removes a lot of Moment's elegance.

@icambron
Copy link
Member

@icambron icambron commented Apr 28, 2014

@Meekohi We'll always allow ISO strings like '2013-05-10', which IMO everyone should just use. As for other strings, '05/10/2013' works differently in different locales, and most of the rest don't work across browsers. These aren't really side-cases; we receive an awful lot of support tickets here about surprises from things like moment("1.10.2014") where the user is expecting us to parse it consistently.

We are allowing this to be pluggable, so you can override the fallback parser (i.e. the thing currently giving a deprecation warning and then using the native parser) to do anything you'd like, and I suspect there will be some plugins built that do just that.

@Meekohi
Copy link

@Meekohi Meekohi commented Apr 28, 2014

@icambron Is the correct way to plugin your own parser to overwrite createFromInputFallback? i.e.

moment.createFromInputFallback = function(config) {
  // unreliable string magic, or
  config._d = new Date(config._i);
};
@icambron
Copy link
Member

@icambron icambron commented Apr 28, 2014

@Meekohi Correct.

@hengkiardo
Copy link

@hengkiardo hengkiardo commented Apr 29, 2014

hi guys,
i'm use moment on my node apps.
i using it something like this one

moment(Date('2014-04-21T05:29:59Z')).format("DD-MM-YYYY")

now when i check log on my server, i get error message on my log files

Deprecation warning: moment construction falls back to js Date. This is discouraged and will be removed in upcoming major release. Please refer to https://github.com/moment/moment/issues/1407 for more info.
@icambron
Copy link
Member

@icambron icambron commented Apr 29, 2014

@Aredo I think you're expecting Date(string) to return a Date? It appears to return a string. I think you want moment(new Date(string)) or moment(Date.parse(string)), or even just moment('2014-04-21T05:29:59Z')

@thomasbird1984
Copy link

@thomasbird1984 thomasbird1984 commented May 1, 2014

I'm using an iso 8601 format like this: 2014-04-29 17:47:12, is there a list of the supported formats that will not be deprecated?

@icambron
Copy link
Member

@icambron icambron commented May 1, 2014

(Editing because I misunderstood the question and this is kind of an important doc now). @godoploid Only ISO 8601 are asp.net-JSON-style dates are not deprecated. On your specific format, that isn't ISO-compliant. Use a "T" instead of a space, as in "2014-04-29T17:47:12" (see here).

@thomasbird1984
Copy link

@thomasbird1984 thomasbird1984 commented May 1, 2014

@icambron Got it thanks, I'll make sure to do so!

@imsobear
Copy link

@imsobear imsobear commented May 13, 2014

@icambron
But how I format the current time?

When i try this var now = moment().format('YYYY-M-D'), it warning me: moment construction falls back to js Date. This is discouraged and will be removed in upcoming major release. Please refer to https://github.com/moment/moment/issues/1407 for more info.

@ichernev
Copy link
Contributor Author

@ichernev ichernev commented May 13, 2014

@imsobear I'm not sure the error message you see is from moment().format('YYYY-M-D'), I think its something before that.

@favio41
Copy link

@favio41 favio41 commented May 16, 2014

The documentation have a example that throws this Deprecation message.

Yeah I know, there is "warning" text, but if is for remove, maybe some "Deprecation" in the docs, could help the newbies.

moment("Dec 25, 1995")  -> "moment construction falls..."

http://momentjs.com/docs/#/parsing/string/

glasnt added a commit to WhereSoftwareGoesToDie/machiavelli that referenced this issue May 23, 2014
moment.js deprecation warning and related issues: moment/moment#1407
@cspotcode
Copy link

@cspotcode cspotcode commented Sep 24, 2015

@pmolaro: Moment can do that, too. You just need to give it a format string. For example:

var d = moment(data, 'MMMM Do YYYY, h:mm:ss a'); // don't forget your format string
return (d.isValid() ? d.format('MM/DD/YYYY h:mm A') : ' -- ');

You can specify an array of formats, and moment will try them all.

@jetzhou
Copy link

@jetzhou jetzhou commented Oct 6, 2015

For everyone following @cspotcode's workaround, note that the second argument should actually be moment.ISO_8601, ie

moment("invalid-date", moment.ISO_8601).isValid()==false

relating to #2036

@auluckh23
Copy link

@auluckh23 auluckh23 commented Oct 7, 2015

What if my date is in this format: "2015-10-06 00:00:00.0" and in my function I am doing this:

moment("2015-10-06 00:00:00.0").format("D MMM YYYY");

Is this fine or will it stop working?

@nitulkukadia
Copy link

@nitulkukadia nitulkukadia commented Oct 19, 2015

@ichernev Could you please add the final conclusion, As comments in this issue is too long.

@adrianaguirre
Copy link

@adrianaguirre adrianaguirre commented Oct 22, 2015

I'm sorry guys I'm having a hard time understanding. How can I modify this code so that I am compliant?

var Benchmark = require('benchmark'),
    moment = require("./../moment.js"),
    base = moment('2013-05-25');

module.exports = {
  name: 'clone',
  onComplete: function(){console.log('done');},
  fn: function(){base.clone();},
  async: true
};
@warrendodsworth
Copy link

@warrendodsworth warrendodsworth commented Oct 23, 2015

By using the default JavaScript Date constructor to construct your date object before sending it in to moment. So moment doesn't have to be responsible for parsing your Date string.

moment(new Date('2013-05-25'));
@ipsita93
Copy link

@ipsita93 ipsita93 commented Oct 27, 2015

I get the same error as @imsobear. I am trying to convert from "9:00 AM" format to "00:00:00" format so I can store it as a TIME type in MySQL database. But how to do so?

moment("9:00 AM").format("HH:mm:ss");

Deprecation warning: moment construction falls back to js Date. This is discouraged and will be removed in upcoming major release. Please refer to https://github.com//issues/1407 for more info. Error at Function.createFromInputFallback (http://localhost:3000/javascripts/vendor/moment.js:746:36) at configFromString (http://localhost:3000/javascripts/vendor/moment.js:826:32) at configFromInput (http://localhost:3000/javascripts/vendor/moment.js:1353:13) at prepareConfig (http://localhost:3000/javascripts/vendor/moment.js:1340:13) at createFromConfig (http://localhost:3000/javascripts/vendor/moment.js:1307:44) at createLocalOrUTC (http://localhost:3000/javascripts/vendor/moment.js:1385:16) at local__createLocal (http://localhost:3000/javascripts/vendor/moment.js:1389:16) at utils_hooks__hooks (http://localhost:3000/javascripts/vendor/moment.js:16:29) at Object.$.validate.submitHandler (http://localhost:3000/javascripts/addstudentform.js:620:35) at d (http://localhost:3000/javascripts/vendor/jquery.validate.min.js:4:885) addstudentform.js:621 Invalid date
@iamstarkov
Copy link

@iamstarkov iamstarkov commented Nov 8, 2015

I started getting deprecation warning, but cannot see how I can use both locale and strictness without using this going-to-be-deprecated feature:

moment('23 December 2015', 'DD MMMM YYYY', 'en', true).isValid();

@ichernev what is the migration path for this feature?

@adamreisnz
Copy link

@adamreisnz adamreisnz commented Nov 10, 2015

I seem to be getting this warning after a moment object is coerced into a string. The resulting string becomes %222015-11-11T10:59:59.999Z%22, which corresponds to "2015-11-11T10:59:59.999Z". Isn't a moment supposed to convert itself to a string into a format which would be validated by the moment constructor itself?

Is moment adding those quotes or is that coming out of the toString() method like that?

@mj1856
Copy link
Contributor

@mj1856 mj1856 commented Nov 11, 2015

@adambuczynski looks like url encoding. Moment won't do that, must be coming from elsewhere.

@mj1856
Copy link
Contributor

@mj1856 mj1856 commented Nov 11, 2015

@iamstarkov - That code is fine and does not produce the deprecation error, nor will it be removed.

@mj1856
Copy link
Contributor

@mj1856 mj1856 commented Nov 11, 2015

@ipsita93

moment("9:00 AM","h:mm a").format("HH:mm:ss");
@iamstarkov
Copy link

@iamstarkov iamstarkov commented Nov 11, 2015

@mj1856 actually, its producing error, while im using momentjs in get-md-date in exactly that way

@mj1856
Copy link
Contributor

@mj1856 mj1856 commented Nov 11, 2015

@warrendodsworth - yuck - no, please avoid that. The Date constructor will parse differently depending on implementation. In most current browsers, moment(new Date('2013-05-25')) would produce the local equivalent of the UTC midnight of that date, similar to if you used moment.utc('2013-05-25').local().

Just do moment('2013-05-25'), which is not deprecated. Or, to be absolutely certain, use moment('2013-05-25','YYYY-MM-DD'), which will do the same thing.

@mj1856
Copy link
Contributor

@mj1856 mj1856 commented Nov 11, 2015

@adrianaguirre - Your code is already compliant, at least the code you showed here. 2013-05-25 is one of the recognized ISO 8601 formats.

@mj1856
Copy link
Contributor

@mj1856 mj1856 commented Nov 11, 2015

@auluckh23 - Yes, that is fine. Fractional seconds are valid by ISO8601.

@mj1856
Copy link
Contributor

@mj1856 mj1856 commented Nov 11, 2015

@pmolaro, and all others trying to just remove the deprecation warning. Yes, moment(new Date(string)) will stop the deprecation warning, but then you are entirely missing the point.

The point is, parsing dates via the Date object is unreliable and inconsistent across platforms. There are different implementations and different behaviors. Sometimes you may get local time. Sometimes you may get UTC. Sometimes you may get Invalid Date. Sometimes you will get different results near DST transitions.

Don't try to defeat the error. Instead, follow our guidelines. Either use a known format or supply a format string.

The known formats are the ISO8601 forms listed here, and the older ASP.NET JSON Date form shown here. Anything else needs a format string.

@moment moment locked and limited conversation to collaborators Nov 11, 2015
@mj1856
Copy link
Contributor

@mj1856 mj1856 commented Nov 11, 2015

I've locked this thread. If after reading this thread and looking at your code carefully you still don't understand, then chat with me on Gitter. There's nothing more to talk about here. Thanks.

@mj1856 mj1856 removed the todo label Nov 11, 2015
santigimeno referenced this issue in santigimeno/cron-parser Feb 29, 2016
spathon referenced this issue in sequelize/sequelize Aug 24, 2017
debug@3.0.0
validator@8.0.0
inflection@1.12.0
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.