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

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

Fixes: #4230
Signed-off-by: James M Snell [email protected]

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 <[email protected]>

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 <[email protected]> PR-URL: #38642 Reviewed-By: Steven R Loomis <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

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-actionsbot 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 resolvedHide resolved

srl295 reviewed May 12, 2021
View reviewed changes

src/node_env_var.cc Outdated Show resolvedHide resolved

srl295 reviewed May 12, 2021
View reviewed changes

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

jasnell force-pushed the datetime-change-notify branch from f3f8c3a to e59351bCompare 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 cc783d2Compare 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 9d30626Compare 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 311265aCompare 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 resolvedHide 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 resolvedHide resolved

jasnell force-pushed the datetime-change-notify branch from abcc357 to 4793754Compare 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 resolvedHide 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 <[email protected]>

jasnell force-pushed the datetime-change-notify branch from 5fd73e8 to b6a0facCompare 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

Uh oh!

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

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

Uh oh!

Conversation

jasnell commented May 11, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

srl295 commented May 12, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented May 12, 2021

Uh oh!

srl295 commented May 12, 2021

Uh oh!

srl295 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented May 12, 2021

Uh oh!

This comment has been minimized.

srl295 commented May 12, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

srl295 left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

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 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented May 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jasnell commented May 11, 2021•
edited
Loading

srl295 commented May 12, 2021•
edited
Loading

srl295 left a comment •
edited
Loading

srl295 commented May 12, 2021•
edited
Loading

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