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

TimeZone - a plugin with time zone support for parsing and formatting strings #325

Closed
wants to merge 8 commits into from

Conversation

prantlf
Copy link
Contributor

@prantlf prantlf commented Sep 4, 2018

Day.js Time Zone Plugin https://day.js.org/docs/en/timezone/timezone







Attempts to implement #323 by introducing the TimeZone plugin.

It uses the IANA time zone database, packed to a JSON format by moment-time-zone and with a simple interface to it provided by timezone-support.

Although the Intl time zone support has been introduced, there are still browsers or platforms, which do not support it and would not mind "paying for" the support by including more JavaScript code. The size of the time zone supporting library without the Day.js plugin: source 185 kB, minified 182 kB, gzipped 24 kB.

I can imagine adding support for remembering the time zone for future calls to .format() as discussed at #46.

Time zone conversions are possible already without an additional API. Jut use UTC date from one Day.js object and format it the target time zone using the other (or the same) Day.js object.

@prantlf
Copy link
Contributor Author

prantlf commented Sep 4, 2018

The build fails only on Node.js 6, because of the usage of async keyword in build/index.js. @iamkun, do you want to drop the Node.js 6 support, or use only JavaScript features supported by Node.js 6?

$ npm run build
> dayjs@0.0.0-development build /home/travis/build/iamkun/dayjs
> cross-env BABEL_ENV=build node build && npm run size
/home/travis/build/iamkun/dayjs/build/index.js:13
async function build(option) {
      ^^^^^^^^
SyntaxError: Unexpected token function

@codecov-io
Copy link

codecov-io commented Sep 5, 2018

Codecov Report

Merging #325 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #325   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          32     33    +1     
  Lines         386    441   +55     
  Branches       53     63   +10     
=====================================
+ Hits          386    441   +55
Impacted Files Coverage Δ
src/index.js 100% <ø> (ø) ⬆️
src/plugin/timeZone/index.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b14bdd1...505b201. Read the comment docs.

@iamkun
Copy link
Owner

iamkun commented Sep 5, 2018

we build dayjs only on CI (node 10)

@prantlf
Copy link
Contributor Author

prantlf commented Sep 5, 2018

@iamkun , this is your configuration in .travis.yml:

node_js:
  - '10'
  - '8'
  - '6'

I opened PR #327, which drops the Node.js 6 line, for your consideration. Thanks!

Add test for parsing a string with time zone.
@prantlf
Copy link
Contributor Author

prantlf commented Sep 6, 2018

Oh, I am sorry, @iamkun. I implemented other workaround for not having the built files for testing.

I will need to figure out the code coverage yet, which does not show me any report.

@iamkun
Copy link
Owner

iamkun commented Sep 6, 2018

@prantlf you may could run npm run test on your local machine.

@prantlf
Copy link
Contributor Author

prantlf commented Sep 6, 2018

Yes, that helped, thanks! :-) I think, that I confused codecov by rebasing the PR branch to the current master and forcing the re-push.

* Use the timezone-support module to get a lightweight time zone support.
* Construct Dayjs from a zone-less string and an explit time zone.
* Format a string with Dayjs converted to other time zone.
It would be possible to bundle the timezone-support module to the timeZone plugin. It is a single-file dependency.
Upgrade timezone-support to 1.3.0 suporting dayOfWeek.
@codecov-io
Copy link

Codecov Report

Merging #325 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #325   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          32     33    +1     
  Lines         386    440   +54     
  Branches       53     63   +10     
=====================================
+ Hits          386    440   +54
Impacted Files Coverage Δ
src/index.js 100% <ø> (ø) ⬆️
src/plugin/timeZone/index.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b14bdd1...19f3f70. Read the comment docs.

@codecov-io
Copy link

Codecov Report

Merging #325 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #325   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          32     33    +1     
  Lines         386    442   +56     
  Branches       53     63   +10     
=====================================
+ Hits          386    442   +56
Impacted Files Coverage Δ
src/index.js 100% <ø> (ø) ⬆️
src/plugin/timeZone/index.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b14bdd1...19f3f70. Read the comment docs.

3 similar comments
@codecov-io
Copy link

Codecov Report

Merging #325 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #325   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          32     33    +1     
  Lines         386    442   +56     
  Branches       53     63   +10     
=====================================
+ Hits          386    442   +56
Impacted Files Coverage Δ
src/index.js 100% <ø> (ø) ⬆️
src/plugin/timeZone/index.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b14bdd1...19f3f70. Read the comment docs.

@codecov-io
Copy link

Codecov Report

Merging #325 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #325   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          32     33    +1     
  Lines         386    442   +56     
  Branches       53     63   +10     
=====================================
+ Hits          386    442   +56
Impacted Files Coverage Δ
src/index.js 100% <ø> (ø) ⬆️
src/plugin/timeZone/index.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b14bdd1...19f3f70. Read the comment docs.

@codecov-io
Copy link

Codecov Report

Merging #325 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #325   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          32     33    +1     
  Lines         386    442   +56     
  Branches       53     63   +10     
=====================================
+ Hits          386    442   +56
Impacted Files Coverage Δ
src/index.js 100% <ø> (ø) ⬆️
src/plugin/timeZone/index.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b14bdd1...19f3f70. Read the comment docs.

@codecov-io
Copy link

Codecov Report

Merging #325 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #325   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          32     33    +1     
  Lines         386    442   +56     
  Branches       53     63   +10     
=====================================
+ Hits          386    442   +56
Impacted Files Coverage Δ
src/index.js 100% <ø> (ø) ⬆️
src/plugin/timeZone/index.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b14bdd1...a8742af. Read the comment docs.

2 similar comments
@codecov-io
Copy link

Codecov Report

Merging #325 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #325   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          32     33    +1     
  Lines         386    442   +56     
  Branches       53     63   +10     
=====================================
+ Hits          386    442   +56
Impacted Files Coverage Δ
src/index.js 100% <ø> (ø) ⬆️
src/plugin/timeZone/index.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b14bdd1...a8742af. Read the comment docs.

@codecov-io
Copy link

Codecov Report

Merging #325 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #325   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          32     33    +1     
  Lines         386    442   +56     
  Branches       53     63   +10     
=====================================
+ Hits          386    442   +56
Impacted Files Coverage Δ
src/index.js 100% <ø> (ø) ⬆️
src/plugin/timeZone/index.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b14bdd1...a8742af. Read the comment docs.

@codecov-io
Copy link

codecov-io commented Sep 18, 2018

Codecov Report

Merging #325 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #325   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          32     44   +12     
  Lines         386    481   +95     
  Branches       53     65   +12     
=====================================
+ Hits          386    481   +95
Impacted Files Coverage Δ
src/index.js 100% <ø> (ø) ⬆️
src/plugin/timeZone/index.js 100% <100%> (ø)
src/locale/es.js 100% <0%> (ø) ⬆️
src/locale/de.js 100% <0%> (ø) ⬆️
src/locale/fr.js 100% <0%> (ø) ⬆️
src/locale/it.js 100% <0%> (ø) ⬆️
src/locale/ja.js 100% <0%> (ø) ⬆️
src/locale/ru.js 100% <0%> (ø) ⬆️
src/locale/es-es.js 100% <0%> (ø)
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b14bdd1...341d3ba. Read the comment docs.

@prantlf
Copy link
Contributor Author

prantlf commented Sep 18, 2018

@pedro-mass, true, I ignored the day of the week in the original submission. I added it by the latest update. All properties of the days object should be covered now.

@pedro-mass
Copy link

@prantlf This looks awesome, thanks for the hard work and contribution!
Your timezone-support library also seems pretty interesting. Keep up the good work 👏

@prantlf
Copy link
Contributor Author

prantlf commented Sep 29, 2018

Thank you! Let's see how it goes on :-)

I was thinking about introducing an interface compatible with Moment.js:

// Alternative to the constructor:
// dayjs('2018-09-29 21:34', { timeZone: 'Europe/Berlin' })
dayjs.tz('2018-09-29 21:34', 'Europe/Berlin')

// Convert an existing Day.js instance to other time zone
dayjs().tz('Europe/Berlin')

The alternative to the constructor means no problem to the further usage. You might question if it is worth adding, because you need to pass the time zone as a parameter anyway, unlike the static method dayjs.utc(...). Another thing might be the lack of the instance method dayjs().tz(...), because it may not be wanted because of its problems. Anyway, no technical no problems, just some artificial subjective opinions ;-)

The time zone conversion for a Day.js instance may confuse the end-user. Day.js stores a Date instance internally and caches the date and time parts in this.$* properties used by getting and formatting methods. The Date object does not support arbitrary time zones. How to store a date in a particular time zone?

  1. Overhaul Day.js not to use the Date object. The converting method toDate() would return the native Date object in the local time, possibly in a different time zone, than the Day.js instance. This would break the plugins, which depend on this.$d.
  2. Add a new object to store the time-zone-aware date. This would make the Day.js instance more complicated and need some synchronization with this.$d to prevent confusion.
  3. Do not store the time zone information in Day.js instances. Use it only in the constructor/parse method to convert the input to UTC and store it in the Day.js instance, and in the formatting method to output a date string in a request ed time zone. This would not allow to carry the original time zone information together with the date value.

The first approach would be interesting to prototype. It could turn up lightweight and quick. I chose the third approach, because it was not intrusive, it was feasible as a plugin and the interface could be kept if the first approach won later.

However, The third approach does store the time zone information publicly in a Day.js instance, which means, that converting one Day.js instance to another one in an arbitrary time zone is not possible. When I could not add the instance method tz, I did not find adding the static method tz a good idea.

@aralroca
Copy link

aralroca commented Oct 2, 2018

@prantlf Thanks for that great contribution! The interface compatible with Moment.js would be very great. 👏

@prantlf
Copy link
Contributor Author

prantlf commented Oct 7, 2018

@aralroca, I'm afraid, that I went for the option 3 from my comment above, which doesn't allow implementing the interface fully compatible with Moment.js... :'-{

I could implement the static tz method parsing the input in the specified time zone. However, the instance method tz cannot be implemented with the the option 3, because this option doesn't store the time zone in the dayjs object.

The trouble with the static tz from the option 3 is, that it converts the date to the local time zone, which is the only one supported by the native Date object.

const date = dayjs.tz('2018-09-29 21:34', 'Europe/Berlin')
// Some code here, or the date may be passed to a distant function
const output = date.format('DD/MM/YYYY')

What would you expect from such dayjs object? Would you expect the output in the original time zone, or in the local time, as it is done now?

@petermikitsh
Copy link

Hi, I'm inquiring to the status of this PR. Would be incredibly useful to have timezone support in dayjs. Thanks.

Repository owner deleted a comment from pedro-mass Feb 24, 2019
@danielb2
Copy link

curious about status too

@petermikitsh
Copy link

@danielb2 You might not need dayjs. Intl.DateTimeFormat may give you what you need.

https://caniuse.com/#search=datetimeformat

For older browsers, there's a polyfill (npm module intl):

https://bundlephobia.com/result?p=intl@1.2.5

@bissolli
Copy link

@iamkun @prantlf, any news on it? Would be amazing having this plugin.

@trylovetom
Copy link

Any Update? I can't wait for this plugin.

@iamkun
Copy link
Owner

iamkun commented Jul 29, 2019

This pr introduced a heavy dependency which is not really suitable to dayjs. We will introduce a time zone plugin base on Intl.DateTimeFormat.

Day.js Time Zone Plugin https://day.js.org/docs/en/timezone/timezone

@iamkun iamkun closed this Jul 29, 2019
@trylovetom
Copy link

@iamkun so you will adopt #608 ?

@iamkun
Copy link
Owner

iamkun commented Aug 4, 2020

Day.js Time Zone Plugin https://day.js.org/docs/en/timezone/timezone


| Format | Output | Description |
| ------ | ------ | ----------------------- |
| `z` | CEST | Time zone abbreviation |
Copy link

Choose a reason for hiding this comment

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

@iamkun any chance to implement this as part of the timezone plugin?

Copy link
Contributor

Choose a reason for hiding this comment

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

@akoskm I have the same need and draft a PR here #1069

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

Successfully merging this pull request may close these issues.

None yet