Closed db82407 closed 1 year ago
@jakubroztocil would it be possible to get this released 🙏 Thanks!
Would love to see this get merged too 👍
This fix works for me too, hope it get's merged.
We need this correction too , it would be great to get it released :smiley:
I would appreciate if this one gets merged. Is there anything that can be done in order to speed up the merge and the release?
@jakubroztocil is there anything I can help with to get that fix merged? 🙏
@jakubroztocil Can we merge this pull request asap please :pray: Thanks !
Please @jakubroztocil if you can review it and merge. This is realy a bummer for us. It would be a pity to have to fork just for that...
Why en-US
? Will there be any side effects from this choice?
Thanks for contributing this PR!
I'm seeing the test that was introduced failing, even with the code change, so that will need to be fixed before this PR can be merged. I'd like to see a failing test introduced illustrating the problem, then changes that fix that failing test (without changing the behavior of other tests).
Additionally, I'm not convinced this is the right fix:
If the application doesn't provide a locales argument, or the runtime doesn't have a locale that matches the request, then the runtime's default locale is used.
The undefined
argument should use the default system locale. It might be necessary to provide an argument for specifying the locale, but it probably should not be hardcoded in this way.
I'm seeing the test that was introduced failing, even with the code change, so that will need to be fixed before this PR can be merged. I'd like to see a failing test introduced illustrating the problem, then changes that fix that failing test (without changing the behavior of other tests).
Please can you tell me where you see it failing?
If I remove the fix and just include the new test, I see
$ LANG=en_US yarn test
381 passing (1s)
9 pending
and
$ LANG=en_GB yarn test
380 passing (1s)
9 pending
1 failing
1) RRule
testBetweenWithTZ [every week on Tuesday] [DTSTART;TZID=Europe/London:20220613T090000
RRULE:FREQ=WEEKLY;BYDAY=TU]:
TypeError: Cannot read properties of null (reading 'split')
at Object.inspectDate [as Date] (node_modules/loupe/loupe.js:384:36)
That error is caused because the value being split is Invalid Date
.
Adding the fix, the tests work in both locales for me.
Looks like there are some Prettier errors...
Looks like there are some Prettier errors...
Sorry, now fixed. Please approve workflow.
I've fixed the expectedDates for the failing test. I also found that expectedDate() was using similar logic to rezonedDate(), so I extracted a common dateInTimeZone() method.
We could probably also use some new workflows in .github/workflows/nodejs.yml
to run in a few various locales, using the LANG=
env variable...
Rather than adding new workflows, I've just specified LANG corresponding to the specified TZ. I've enabled workflows on my fork and the updated test matrix runs successfully.
Looks great & works on my machine (and in CI!). Thanks for your contribution!
Glad to help.
fixes https://github.com/jakubroztocil/rrule/issues/523
The problem was introduced in https://github.com/jakubroztocil/rrule/pull/508 by commit: https://github.com/jakubroztocil/rrule/commit/cf76af345f02dd1122a432acb5a85a7236b99228 in
src/datewithzone.ts
:This relies on
new Date()
parsing the output ofdate.toLocaleString()
, but it only recognizes month/day/year, i.e. US date format. As the first parameter ofdate.toLocaleString()
isundefined
, the output will be in the host's locale. As I'm in London, that is en-GB which formats the date as day/month/year which causesnew Date()
to returnInvalid Date
.The solution is to explicitly specify the
en-US
locale instead ofundefined
.Thanks for contributing to
rrule
!To submit a pull request, please verify that you have done the following:
master
commit