lib: support setting process.env.TZ on windows #38642

jasnell · 2021-05-11T23:58:34Z

Fixes: #4230
Signed-off-by: James M Snell jasnell@gmail.com

Update: Even tho this is semver-patch, I'm marking it as a notable change for when it lands. The bug was first reported back in Dec 2015, it was closed for a while, one partial attempt at fixing it was made a couple of years ago, and this completes the fix.

/cc @targos @srl295

src/node_env_var.cc

test/parallel/test-datetime-change-notify.js

srl295 · 2021-05-12T00:43:04Z

Might just have NODE_DEFAULT_ZONE instead of trying to emulate TZ. don't use abbrevs just tz IDs like America/Los_Angeles.

jasnell · 2021-05-12T02:25:08Z

@srl295:

Might just have NODE_DEFAULT_ZONE instead of trying to emulate TZ.

In the current version of the PR, use of set TZ="..." does work. Is there a particular limitation here that would make it undesirable? What would using NODE_DEFAULT_ZONE give us? My concern would be introducing something that is inconsistent with TZ on non-windows systems.

srl295 · 2021-05-12T02:27:41Z

@srl295:

Might just have NODE_DEFAULT_ZONE instead of trying to emulate TZ.

In the current version of the PR, use of set TZ="..." does work. Is there a particular limitation here that would make it undesirable? What would using NODE_DEFAULT_ZONE give us? My concern would be introducing something that is inconsistent with TZ on non-windows systems.

What matt was alluding to is that it's already inconsistent. Posix does a bunch of other processing. Node-default-zone would make it clear we are simply choosing a new default zone by the full id rather than trying To emulate Posix TZ on windows.

srl295

I'll say lgtm with caveats that it will not be === TZ behavior on posix, suggestions as noted in thread.

jasnell · 2021-05-12T19:01:44Z

I'll say lgtm with caveats that it will not be === TZ behavior on posix, suggestions as noted in thread.

Understood. I think those differences will be ok here. I'll try to find a good place to describe those differences in the docs

srl295 · 2021-05-12T19:19:38Z

I'll say lgtm with caveats that it will not be === TZ behavior on posix, suggestions as noted in thread.

Understood. I think those differences will be ok here. I'll try to find a good place to describe those differences in the docs

one reference: https://www.gnu.org/software/libc/manual/html_node/TZ-Variable.html
ICU won't necessarily handle those completely either, but it will try to find a matching zone.
Perhaps something like this:

While the TZ env variable on Node.js won't handle all of the various ways that the TZ variable is handled in other environments, it will support tz ids (such as Etc/UTC, Europe/Paris or America/New_York. It may support a few other abbreviations or aliases, but these are strongly discouraged.

sorry not sorry for being a stick in the timezone mud :)

srl295

🇮🇪

src/node_i18n.cc

mcollina

lgtm

doc/api/cli.md

Fixes: nodejs#4230 Signed-off-by: James M Snell <jasnell@gmail.com>

nodejs-github-bot · 2021-05-14T23:07:20Z

CI: https://ci.nodejs.org/job/node-test-pull-request/38099/ 💚

jasnell · 2021-05-17T19:03:13Z

Landed in 16cb4f7

Fixes: #4230 Signed-off-by: James M Snell <jasnell@gmail.com> PR-URL: #38642 Reviewed-By: Steven R Loomis <srloomis@us.ibm.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Notable changes: async_hooks: * (SEMVER-MINOR) use new v8::Context PromiseHook API (Stephen Belanger) #36394 lib: * support setting process.env.TZ on windows (James M Snell) #38642 module: * (SEMVER-MINOR) add support for `URL` to `import.meta.resolve` (Antoine du Hamel) #38587 process: * (SEMVER-MINOR) add `'worker'` event (James M Snell) #38659 util: * (SEMVER-MINOR) add util.types.isKeyObject and util.types.isCryptoKey (Filip Skokan) #38619 PR-URL: #38719

nodejs/node#38642

github-actions bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels May 11, 2021

jasnell mentioned this pull request May 12, 2021

I can't set a default timezone on windows. #4230

Closed

jasnell commented May 12, 2021

View reviewed changes

src/node_env_var.cc Outdated Show resolved Hide resolved

srl295 reviewed May 12, 2021

View reviewed changes

src/node_env_var.cc Outdated Show resolved Hide resolved

srl295 reviewed May 12, 2021

View reviewed changes

test/parallel/test-datetime-change-notify.js Outdated Show resolved Hide resolved

jasnell force-pushed the datetime-change-notify branch from f3f8c3a to e59351b Compare May 12, 2021 02:22

jasnell marked this pull request as ready for review May 12, 2021 02:25

srl295 approved these changes May 12, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

jasnell force-pushed the datetime-change-notify branch 2 times, most recently from 3f15012 to cc783d2 Compare May 12, 2021 19:42

srl295 approved these changes May 12, 2021

View reviewed changes

jasnell force-pushed the datetime-change-notify branch from cc783d2 to 9d30626 Compare May 12, 2021 19:50

This comment has been minimized.

Sign in to view

jasnell force-pushed the datetime-change-notify branch from 9d30626 to 311265a Compare May 12, 2021 20:11

srl295 approved these changes May 12, 2021

View reviewed changes

addaleax requested changes May 12, 2021

View reviewed changes

src/node_i18n.cc Outdated Show resolved Hide resolved

mcollina approved these changes May 12, 2021

View reviewed changes

shalvah reviewed May 13, 2021

View reviewed changes

doc/api/cli.md Outdated Show resolved Hide resolved

jasnell force-pushed the datetime-change-notify branch from abcc357 to 4793754 Compare May 13, 2021 03:34

jasnell requested a review from addaleax May 13, 2021 03:35

This comment has been minimized.

Sign in to view

addaleax approved these changes May 13, 2021

View reviewed changes

doc/api/cli.md Outdated Show resolved Hide resolved

srl295 approved these changes May 13, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

lib: support setting process.env.TZ on windows

b6a0fac

Fixes: nodejs#4230 Signed-off-by: James M Snell <jasnell@gmail.com>

jasnell force-pushed the datetime-change-notify branch from 5fd73e8 to b6a0fac Compare May 14, 2021 01:17

This comment has been minimized.

Sign in to view

fixup! lib: support setting process.env.TZ on windows

8f3bbc4

This comment has been minimized.

Sign in to view

srl295 added the i18n-api Issues and PRs related to the i18n implementation. label May 14, 2021

jasnell closed this May 17, 2021

jasnell added the notable-change PRs with changes that should be highlighted in changelogs. label May 17, 2021

targos mentioned this pull request May 18, 2021

v16.2.0 release proposal #38719

Merged

codebytere added a commit to electron/electron that referenced this pull request May 19, 2021

lib: support setting process.env.TZ on windows

0c498e7

nodejs/node#38642

codebytere added a commit to electron/electron that referenced this pull request May 20, 2021

lib: support setting process.env.TZ on windows

e00d861

nodejs/node#38642

targos added the backport-blocked-v14.x label May 30, 2021

codebytere added a commit to electron/electron that referenced this pull request May 31, 2021

lib: support setting process.env.TZ on windows

a34be21

nodejs/node#38642

codebytere added a commit to electron/electron that referenced this pull request May 31, 2021

lib: support setting process.env.TZ on windows

78972e5

nodejs/node#38642

codebytere added a commit to electron/electron that referenced this pull request Jun 8, 2021

lib: support setting process.env.TZ on windows

f0349b7

nodejs/node#38642

codebytere added a commit to electron/electron that referenced this pull request Jun 9, 2021

lib: support setting process.env.TZ on windows

3492dca

nodejs/node#38642

codebytere added a commit to electron/electron that referenced this pull request Jun 10, 2021

lib: support setting process.env.TZ on windows

6766b65

nodejs/node#38642

richardlau mentioned this pull request Jul 16, 2021

test: set locale for datetime-change-notify test #38741

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: support setting process.env.TZ on windows #38642

lib: support setting process.env.TZ on windows #38642

jasnell commented May 11, 2021 •

edited

srl295 commented May 12, 2021 •

edited

jasnell commented May 12, 2021

srl295 commented May 12, 2021

srl295 left a comment •

edited

jasnell commented May 12, 2021

This comment has been minimized.

srl295 commented May 12, 2021 •

edited

srl295 left a comment

This comment has been minimized.

mcollina left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented May 14, 2021 •

edited by jasnell

jasnell commented May 17, 2021

lib: support setting process.env.TZ on windows #38642

lib: support setting process.env.TZ on windows #38642

Conversation

jasnell commented May 11, 2021 • edited

srl295 commented May 12, 2021 • edited

jasnell commented May 12, 2021

srl295 commented May 12, 2021

srl295 left a comment • edited

Choose a reason for hiding this comment

jasnell commented May 12, 2021

This comment has been minimized.

srl295 commented May 12, 2021 • edited

srl295 left a comment

Choose a reason for hiding this comment

This comment has been minimized.

mcollina left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented May 14, 2021 • edited by jasnell

jasnell commented May 17, 2021

jasnell commented May 11, 2021 •

edited

srl295 commented May 12, 2021 •

edited

srl295 left a comment •

edited

srl295 commented May 12, 2021 •

edited

nodejs-github-bot commented May 14, 2021 •

edited by jasnell