-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
feat: add parse zone plugin #2060
base: dev
Are you sure you want to change the base?
Conversation
@iamkun let me know if you need me to do anything else, or add docs, I'm happy to progress this quickly. Having this plugin as part of dayjs, I think we would be able to catch problems and benefit from the work people can put in OSS, rather than copying and pasting this code everywhere like the issue seem to imply... |
@@ -0,0 +1,69 @@ | |||
const REGEX_TIMEZONE_OFFSET_FORMAT = /^(.*)([+-])(\d{2}):(\d{2})|(Z)$/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this would also allow something like +44:00
- not a biggie, but maybe this could be changed to ^(.*)([+-])([0-2]\d{1}):([0-5]\d{1})|(Z)$
so it only allows possible timezones?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting one here!
So, dayjs
does its own validation on this, but not sure at what point it would fail (I've seen it's mainly when you try to format or using a getter).
I think we need some direction from @iamkun here to advice on the best approach, mainly as if we update the matching pattern and it doesn't match, it would just fallback to the default constructor, whereas now it passes the offset to the utc plugin (which I would expect to deal with).
I think we're just going to publish this internally to use as we need it, but I'll leave the PR open hoping eventually it will get merge or reviewed :) |
@vronifuchsberger @abdirahmanjama guys, any ETA on when these changes will be released? it's a big pain :( |
not a maintainer, so unfortunately can't help you getting this in - we ended up publishing it internally in our company and re-use it in our repos until this (hopefully) get's merged 😄 |
It would be amazing to have this released. For now, I will also try to use it internally :) Thank you for your effort guys! |
This looks awesome! |
Thank you all! Please release this 🤕 |
really looking for this one. |
This is an essential feature for a library trying to be a replacement for moment! I'm going to have to use this internally until this is included in an official release. |
ef415ac
to
5c254dc
Compare
Would love to have this in dayjs! One thing I noticed when trying this out (which might well be a bug in the const str = '2018-11-13T14:54:59Z'
const value = dayjs.parseZone(str)
expect(value.offsetName()).toEqual('UTC') // <-- this fails, it is my local timezone (currently GMT+1)
expect(value.format()).toEqual(str) |
I would love to see this in dayjs, will follow this. @LucaColonnello Just FYI, this regex makes double colons optional in the offset. Just in case you mind in giving that support. /^(.*)([+-])(\d{2})(?::?(\d{2}))|(Z)$/ Will support both 2013-01-01T00:00:00-13:00 as well as 2013-01-01T00:00:00-1300 as valid dates to parseZone |
It would be so great to merge this plugin. Thanks everyone for your efforts! |
This is great. |
why is this still not merged? |
Great addition! Currently also using this internally; would be great if this could become part of the next release. |
Closes #651
I'm opening this PR based on the discussion in the linked issue above.
This is something that affects many people as timezone is a complicated topic.
Example scenarios are:
Example usage: