Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

RangeError when creating a moment from a cloned object #1404

Closed
chilversc opened this Issue · 6 comments

4 participants

@chilversc

If you clone a moment using other libraries such as underscorejs' clone method, or knockoutjs' toJS method and then try to convert this back to a moment you get an infinite recursion.

Steps to reproduce

moment(_.clone(moment()));

Example

A more realistic case where this happens is if you use something like knockout.postbox which internally calls JSON.stringify(ko.toJS(value)).

  1. ko.toJS clones the moment to an object but retains the toJSON method.
  2. JSON.stringify then tries to call the toJSON method.
  3. Moment's toJSON method then tries to clone this before converting to utc. Unfortunately this is no longer a moment.

The code for this would look something like this.

var m = new moment();
var data = {when: m};
ko.postbox.publish('something.happened', data);

This makes it far less obvious that the moment has been cloned to a plain object.

Expected behaviour

Either moment should support moments that have been cloned incorrectly by other libraries, or a more meaningful exception should be thrown rather than causing a stack overflow.

@ichernev
Owner

The problem is that the class is not correct -- so we don't detect it as being a moment object in the first place. Not to mention that all of the prototype stuff is poured inside (these people haven't heard about hasOwnProperty apparently).

I guess we can work around that and make sure that if you pass a badly cloned moment object to a constructor, it should produce a nice moment object, but I wouldn't suggest using the bad clone for anything except formatting (not sure if even that is safe).

@icambron
Collaborator

Formatting isn't save either, actually, since some of the formatting methods clone internally.

@icambron
Collaborator

Why don't we just add a property called _isAMomentObject and test for it?

@mattbrooks2010

This would only work for the KO case if _isAMomentObject is a function.

@ichernev
Owner

OK, so lets say we detect it somehow

if (moment.isMoment(input)) {
    config = extend({}, input);
    config._d = new Date(+input._d);
}

So I guess we need to only take the relevant fields (the ones with the underscore), because right now _.clone also fetches the functions from the prototype.

I just checked on a page with ko.toJS, I put a null property with underscore, and toJS preserved it. So I guess we need to 1) put a fancy property inside and 2) clone only known (whitelisted) properties.

@ichernev
Owner

Fixed in #1417

@ichernev ichernev closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.