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

Remove date auto type conversions #5982

Merged
merged 1 commit into from
Jan 17, 2019
Merged

Conversation

benmccann
Copy link
Contributor

@benmccann benmccann commented Jan 15, 2019

These were mostly no-ops now that we have timestamps

new Date().getTime() is more than twice as fast as +new Date() and Date.now() faster still. https://jsperf.com/new-date-timing

It also appears to be faster not to use +moment as well. https://jsperf.com/moment-getvalue

Related to #5960

etimberg
etimberg previously approved these changes Jan 15, 2019
Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 small comments

src/scales/scale.time.js Outdated Show resolved Hide resolved
src/scales/scale.time.js Outdated Show resolved Hide resolved
Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I agree with the changes in adapter.moment.js (and Date.now()), I don't think there is any good reason to remove the + to cast to number in scale.time.js.

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

Successfully merging this pull request may close these issues.

4 participants