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

Regression on Date.getHours() on node v10.4.1 #21308

Closed
germain-gg opened this issue Jun 13, 2018 · 8 comments
Closed

Regression on Date.getHours() on node v10.4.1 #21308

germain-gg opened this issue Jun 13, 2018 · 8 comments
Labels
v8 engine Issues and PRs related to the V8 dependency.

Comments

@germain-gg
Copy link

  • Version: 10.4.1
  • Platform: Darwin 17.5.0 Darwin Kernel Version 17.5.0: Fri Apr 13 19:32:32 PDT 2018; root:xnu-4570.51.2~1/RELEASE_X86_64 x86_64

I have recently upgraded to node v10.4.1, we were running on v10.3.0 before but decided to upgrade due to https://nodejs.org/en/blog/vulnerability/june-2018-security-releases/

It seems like there is a regression on the Date object with timezone.

const d1 = new Date(1);
console.log(d1.getHours()); // outputs 1

The expected output is 0. When running v10.3.0, the code above outputs 0.

I am located in London, which means that my timezone is BST (British Summer Time). My computer date/time is correct, it is connected to Apple's NTP server (time.euro.apple.com)

Not sure what other info would be relevant for you to solve this issue. Let me know if you need anything from me.

@addaleax addaleax added the v8 engine Issues and PRs related to the V8 dependency. label Jun 13, 2018
@addaleax
Copy link
Member

@nodejs/v8

@addaleax
Copy link
Member

Also, this is an issue with v10.4.0 as well, so it’s not the V8 timezone patch we included in v10.4.1.

@bnoordhuis
Copy link
Member

We upgraded from V8 6.6 to 6.7 and 6.7 has correcter timezone handling (it delegates it to ICU.)

The expected output is 0.

Can you explain why?

@germain-gg
Copy link
Author

@bnoordhuis I don't really have any data to back this up, but it is the behaviour in a lot of previous node versions, I just tested v10.3.0, v9.5.0, v8.0.0 and they all output 0. Hence me assuming that it is the expected output.

@AyushG3112
Copy link
Contributor

AyushG3112 commented Jun 14, 2018

According to the MDN docs:

The getHours() method returns the hour for the specified date, according to local time.

Since BST(British Summer Time) is UTC + 1, new Date(1).getHours() most probably adds your timezone offset hours to give you 1.

To get the UTC hours without the timezone offset, you should use getUTCHours instead of getHours.

@germain-gg
Copy link
Author

@AyushG3112 Yep definitely agree with you, getHours() is locale dependent. However What puzzles me is that every older version has a different behaviour?

@AyushG3112
Copy link
Contributor

AyushG3112 commented Jun 14, 2018

@gsouquet According to what @bnoordhuis said

We upgraded from V8 6.6 to 6.7 and 6.7 has correcter timezone handling (it delegates it to ICU.)

And V8 6.7 landed in Node.js v10.4.0, you can see the relevent PR for V8 here: #19989

This means that any release before v10.4.0 handles timezones differently than v10.4.0 and later do.

@Debugreality
Copy link

As an extra note I'm using Node 8.11 in UTC+10 and getHours() returns one hour more than it should, toLocaleString() returns the correct time - this is from a date initiated with an ISODate string:

New Date(new Date().toISOString()).getHours() - 17
New Date(new Date().toISOString()).toLocaleString() - '2018-6-19 16:05:41'

The above code in Chrome returns 16.

Hopefully v10.4 will fix this when I get around to upgrading!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

No branches or pull requests

5 participants