-
Notifications
You must be signed in to change notification settings - Fork 156
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
fix(expression) handle canonical time zone designations correctly #272
Conversation
From the CI, it looks like the new system test fails for Node 12 only (passing for Node 14, 15, 16, and 17). The failure for Node 12 is the same before and after the other changes in this PR, which I think shows that this PR fixes this issue for Node versions 14 and newer and makes it no worse for Node version 12. |
test/expression.js
Outdated
test('it will work with #271 issue case for Node v > 12', function(t) { | ||
if(process.versions.node.startsWith('12.')) { | ||
t.end(); | ||
return; | ||
} | ||
var options = { | ||
currentDate : new Date('2022-01-01T00:00:00.000Z'), | ||
tz: 'EST5EDT' | ||
}; | ||
|
||
var interval = CronExpression.parse('0 0 5 * * *', options); | ||
var date = interval.next(); | ||
|
||
t.equal(date.toISOString(), '2022-01-01T10:00:00.000Z'); | ||
|
||
t.end(); | ||
}); |
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.
If you would prefer that we bump to Luxon v 2 but do not add this test case, please let me know and I can remove this new test.
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.
@DonBrinn yes, let's remove it and just proceed with package version bump.
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.
Thanks! Will merge and release it tomorrow (as a new minor version).
Fixes issue #271.
The new unit test was added first, as test-driven development. The new unit test failed with the following error before package.json was updated.
After package.json was updated to use the newer version of Luxon, the new unit test succeeded.
The results of all other tests were unchaged by using the new version of Luxon.