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

Rethink _d #2617

Closed
mattjohnsonpint opened this issue Sep 17, 2015 · 7 comments
Closed

Rethink _d #2617

mattjohnsonpint opened this issue Sep 17, 2015 · 7 comments

Comments

@mattjohnsonpint
Copy link
Contributor

I'm wondering if we really need to track a Date object in _d.

  • Loads of questions are raised from folks logging a moment instance to the console, seeing _d and not understanding how to interpret it. While ._ convention is common for internal/private fields, it's still not something the common user understands. Also in the console, the string it displays is implementation dependent. Most browsers show the local time, but others (like Firefox) show the UTC time. So a lot of misunderstandings are when the string representation of _d doesn't align with the date emitted by .format().
  • Currently, the value of _d.valueOf() is the same as moment.valueOf() only when the moment is either local or UTC. As soon as there is an offset set in _offset, then the _d value no longer represents the same instant in time, but rather the instant shifted by the offset. Our valueOf() function (and many others) have to take _offset into account and shouldn't. This is both an efficiency problem, and an easy source of errors. It also isn't understood when users look at _d in isolation (per my first point).
  • It would make a lot more sense (IMHO) to keep just a timestamp as a number, which is always the UTC-based timestamp of the specific instant in time that the moment is about. Perhaps in _ts (for timestamp). The _offset would be applied to _ts for the functions that need it.
  • We may find we can actually get rid of several functions that use the Date object. For example, I have recently re-implemented functions for date-part to timestamp and vice-versa in pure JavaScript, and they perform faster than native code. This could be pulled into moment, and other similar functions can probably be written that manipulate the timestamp directly rather than calling functions like setMonth() on the Date object. This would also have the benefit of not being browser-dependent on DST transition adjustment behavior.
  • I think this will also make it easier for some of the moment-timezone integration.
@ichernev
Copy link
Contributor

I wouldn't base my decision on folks debugging moment and not understanding how it works.

About the performance increase -- you can convert some methods to use timestamp but not all, unless you store a date object yourself and do all the nasty things Dates do when you set month, day, hour etc. I actually started a pure javascript Date implementation that was more predictable, but I haven't made progress on it for some time.

All of the set methods will be super slow if you just do timestamp, because you have to explode to fields, then manipulate them and then convert back to timestamp. If you store fields directly it uses more memory.

https://github.com/ichernev/utc_date if you want to work on that, be my guest :) At the least we need to implement (almost) all Date manipulation methods (mostly getters, setters, convert to unix, no parsing), and a way to plug DST source (moment-timezone IANA timezone DB or Date (for local zones only)). That is actually mostly done in the current code.

Handling DST on creation is not done yet -- it has to decide what to do if its a hole (no possible UTC), if its an overlap (2 possible UTC values for one local one), possibly via configuration / callbacks (configurable defaults).

I still haven't decided how to handle DST weird cases in general -- we can have a config on what to do, but also callbacks that will notify the user of these things happening (some might use that). Its just complicated :)

@jzabroski
Copy link

What is the difference between _i and _d? I have seen many situations where these values don't match under clone() in 2.5.2 and was digging through _d and discovered it called the underlying new Date() constructor, and then found this thread. I am wondering if there is some documentation on these private fields somewhere...?

@mattjohnsonpint
Copy link
Contributor Author

@jzabroski - I wholeheartedly agree that they are underdocumented. They shouldn't be in the public API docs, but should be on a developer/contributor doc page somewhere.

The i in _i stands for input. Whenever you create a moment by passing it something, that gets copied into _i. I'm quite sure why this is necessary though. _i might contain a string, a number, a Date object, or an array, or it might not exist at all if you just use moment().

_d is the Date object underlying the moment. It is sometimes read directly, and sometimes combined with _offset - depending on the circumstances.

@jzabroski
Copy link

A good minifier is supposed to eliminate comments from moment.js proper. There is no reason for a separate "doc page".

@mattjohnsonpint
Copy link
Contributor Author

@twinklebob
Copy link

SIgned up to CodeTriage.com to make the best of some unexpected free time and it sends me this as my first go. 😂 As there's no activity since 2015 could this issue be closed?

@marwahaha
Copy link
Member

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

No branches or pull requests

5 participants