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

NodeJS reports incorrect time zone for Brazil #30211

Closed
albertyw opened this issue Nov 1, 2019 · 13 comments
Closed

NodeJS reports incorrect time zone for Brazil #30211

albertyw opened this issue Nov 1, 2019 · 13 comments

Comments

@albertyw
Copy link
Contributor

albertyw commented Nov 1, 2019

  • Version: v13.0.1
  • Platform: Darwin 18.7.0 Darwin Kernel Version 18.7.0: Tue Aug 20 16:57:14 PDT 2019
  • Subsystem: N/A

Node is reporting the wrong time zone for Brazil after its cancelled daylight savings change on November 2.

Reproduction:

var process = require('process');
console.log('ICU version: ' + process.versions.icu);

var options = {
    timeZone: "America/Sao_Paulo",
    year: 'numeric', month: 'numeric', day: 'numeric',
    hour: 'numeric', minute: 'numeric', second: 'numeric',
    timeZoneName: 'short'
};

var formatter = new Intl.DateTimeFormat([], options);

var UTCTimeBefore = new Date(new Date().toUTCString());
var UTCTimeAfter = new Date(UTCTimeBefore.getTime() + (7 * 24 * 60 * 60 * 1000));
var UTCTimeAfter = new Date(UTCTimeBefore.getTime() + (30 * 24 * 60 * 60 * 1000));
var localTime = formatter.format(UTCTimeBefore);
console.log('Should be GMT-3')
console.log(localTime);
var localTime = formatter.format(UTCTimeAfter);
console.log('Should also be GMT-3')
console.log(localTime);

Outputs:

ICU version: 64.2
Should be GMT-3
11/1/2019, 7:19:15 PM GMT-3
Should also be GMT-3
12/1/2019, 8:19:15 PM GMT-2

I believe this is because node v13.0.1 depends on ICU version 64.2 which includes tzdata 2019a. However, brazil's updated daylight savings was added in tzdata 2019b, which requires a minimum ICU version of 65.1.

@targos
Copy link
Member

targos commented Nov 1, 2019

Tracking issue for update to ICU 65.1: #29540

@apaprocki
Copy link
Contributor

Time zone data files are updated independently of ICU. You do not need to tie updating time zone data with upgrading ICU, which could have other consequences.

To update the existing ICU 64 .dat file:

  • decompress the icudt64l.dat archive
  • download the 2019c files from https://github.com/unicode-org/icu-data/tree/master/tzdata/icunew (le little-endian, and be big-endian, depending on platform)
  • follow the instructions at http://userguide.icu-project.org/datetime/timezone to patch the .dat file for all four time zone resource files (the resource files must be in the current directory):

    icupkg -a <resourceFile> icutdt64l.dat

  • if you want, verify that there is only one of each file in the .dat file using the -l list option
  • if you want, verify that after you -x extract the file, the shasum matches what you intended to patch
  • compress the icudt64l.dat archive as before

To test the patched file works...

Existing Node v12.13.0 release:

$ node -e 'console.log(Intl.DateTimeFormat("en-US", { dateStyle: "long", timeStyle: "long", timeZone: "America/Sao_Paulo" }).format(Date.UTC(2019, 10, 4, 23)))'
November 4, 2019 at 9:00:00 PM GMT-2

After patching .dat file:

$ node -e 'console.log(Intl.DateTimeFormat("en-US", { dateStyle: "long", timeStyle: "long", timeZone: "America/Sao_Paulo" }).format(Date.UTC(2019, 10, 4, 23)))'
November 4, 2019 at 8:00:00 PM GMT-3

If it helps, the shasum of the v12.13.0 release and patched files:
Release: 2b681d53f193f714db86b038176da6866f8d5afd icudt64l.dat
Patched: 7317c1d424812d9ef1ed318e70db7ed27b93853c icudt64l.dat

@MylesBorins
Copy link
Contributor

/cc @srl295

@srl295
Copy link
Member

srl295 commented Nov 11, 2019

@apaprocki would you like to make a PR to update tools/icu/ with a readme about how to do this? Good instructions. I would add that you can have the updated tz data outside of node.js, without recompiling, i will have to add detailed instructions for that.

additionally, process.versions.tz will return the tz version such as 2019b

@apaprocki
Copy link
Contributor

I would add that you can have the updated tz data outside of node.js, without recompiling, i will have to add detailed instructions for that.

I specifically left that part out because Node does not currently build with the external directory enabled. If someone decides on an installation path where Node should look for updated ICU time zone data files, I can include that text as well. (e.g., PREFIX/share/icu/tzdata?)

Also, current ICU (65.1) does not expose the ..._ENV_VAR override for the external time zone resource directory the same way that it does for the whole ICU data file directory. I had to add patch in support for that to ICU to make it so that both data file directories could be properly relocatable with the same (in my case) environment variable. I'll look into sending that patch upstream.

@srl295
Copy link
Member

srl295 commented Nov 12, 2019 via email

@apaprocki
Copy link
Contributor

@srl295 I filed ICU-20895 and opened the pull request for the environment variable.

@MylesBorins
Copy link
Contributor

I've backported to 12.x and 10.x. For now I didn't update 8.x. @nodejs/lts thoughts?

targos pushed a commit that referenced this issue Nov 14, 2019
Refs: #30211
Refs: #30356

PR-URL: #30478
Reviewed-By: Steven R Loomis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
MylesBorins pushed a commit that referenced this issue Nov 17, 2019
Fixes: #30211
PR-URL: #30356
Reviewed-By: Steven R Loomis <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
albertyw added a commit to albertyw/node-1 that referenced this issue Nov 22, 2019
Update the version of the bundled ICU (deps/icu-small) to ICU version
65.2.

Fixes: nodejs#30211
Fixes: nodejs#29540
BethGriggs pushed a commit that referenced this issue Dec 3, 2019
Refs: #30211
Refs: #30356

PR-URL: #30479
Reviewed-By: Steven R Loomis <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
richardlau pushed a commit that referenced this issue Dec 6, 2019
Update the version of the bundled ICU (deps/icu-small) to ICU version
65.2.

Fixes: #30211
Fixes: #29540

PR-URL: #30232
Reviewed-By: Steven R Loomis <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
targos pushed a commit that referenced this issue Dec 9, 2019
Update the version of the bundled ICU (deps/icu-small) to ICU version
65.2.

Fixes: #30211
Fixes: #29540

PR-URL: #30232
Reviewed-By: Steven R Loomis <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
MylesBorins added a commit that referenced this issue Dec 17, 2019
Refs: #30211
Refs: #30356

PR-URL: #30479
Reviewed-By: Steven R Loomis <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
apaprocki added a commit to apaprocki/node that referenced this issue Jan 19, 2020
Updates the "Maintaining ICU" document and describes the process
to update an existing ICU `.dat` file with updated binary time
zone data files corresponding to new IANA `tzdata` releases.

Requested in nodejs#30211 by @srl295
albertyw added a commit to albertyw/node-1 that referenced this issue Jan 21, 2020
Update the version of the bundled ICU (deps/icu-small) to ICU version
65.2.

Fixes: nodejs#30211
Fixes: nodejs#29540
Backport-PR-URL: nodejs#31434
PR-URL: nodejs#30356
PR-URL: nodejs#30232
jameshilliard pushed a commit to jameshilliard/node that referenced this issue Feb 17, 2020
Update the version of the bundled ICU (deps/icu-small) to ICU version
65.2.

Fixes: nodejs#30211
Fixes: nodejs#29540

PR-URL: nodejs#30232
Reviewed-By: Steven R Loomis <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
MylesBorins pushed a commit that referenced this issue Apr 1, 2020
Update the version of the bundled ICU (deps/icu-small) to ICU version
65.2.

Fixes: #30211
Fixes: #29540

Backport-PR-URL: #31433
PR-URL: #30232
Reviewed-By: Steven R Loomis <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
@leonetosoft
Copy link

I have the same problem, I need to set the time zone for the America/Campo_Grande node, I already changed the environment variable (TZ) but it still doesn't work ...

@kirliam
Copy link

kirliam commented May 7, 2020

@leonetosoft I had the same problem, and the workaround I'm using is the "set-tz" package. Install it and add in the first lines of your app:

const setTZ = require('set-tz');
setTZ('America/Bahia');

@srl295
Copy link
Member

srl295 commented May 7, 2020

@kirliam @leonetosoft is this on Windows? That seems to be the only place it would make a difference, all set-tz does is set process.env.TZ besides on Windows.

Edit: i misread, set-tz may not even do that. Can you show me some code that isn't working and tell me the version and OS?

@emarcelino3
Copy link

I substitute timeZone: "America/Sao_Paulo" by timeZone: "-03:00".

work fine to me

@kirliam
Copy link

kirliam commented May 13, 2020 via email

srl295 pushed a commit that referenced this issue May 19, 2020
Updates the "Maintaining ICU" document and describes the process
to update an existing ICU `.dat` file with updated binary time
zone data files corresponding to new IANA `tzdata` releases.

Requested in #30211 by @srl295

PR-URL: #30364
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Steven R Loomis <[email protected]>
codebytere pushed a commit that referenced this issue May 21, 2020
Updates the "Maintaining ICU" document and describes the process
to update an existing ICU `.dat` file with updated binary time
zone data files corresponding to new IANA `tzdata` releases.

Requested in #30211 by @srl295

PR-URL: #30364
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Steven R Loomis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants