Convert to Typescript #228
Conversation
Streamline webpack bundle
package.json
Outdated
| @@ -16,22 +17,41 @@ | |||
| "rfc" | |||
| ], | |||
| "author": "Jakub Roztocil and Lars Schöning", | |||
| "main": "dist/index", | |||
| "main": "dist/es5", | |||
There was a problem hiding this comment.
💛 I believe bower.json will need to be similarly updated to point to the correct distribution
README.md
Outdated
| var RRuleSet = require('rrule').RRuleSet | ||
| var rrulestr = require('rrule').rrulestr | ||
| ```es6 | ||
| import { RRule, RRuleSet, rrulestr } from 'rrule/es6' |
There was a problem hiding this comment.
💛 I don't think its mentioned anywhere in the README but we should point out that you can use
import { RRule, RRuleSet, rrulestr } from 'rrule'
to pull in RRule that requires no transpiling
src/helpers.ts
Outdated
| @@ -15,12 +15,12 @@ const range = function (start, end) { | |||
| return rang | |||
| } | |||
|
|
|||
| const repeat = function (value, times) { | |||
| const repeat = function<T>(value: T | T[], times: number): T[] | T[][] { | |||
There was a problem hiding this comment.
This must have been "fun" to convert to typescript.
| @@ -173,14 +178,14 @@ export const testRecurring = function (msg, testObj, expectedDates) { | |||
| } | |||
| } | |||
| }) | |||
| } | |||
| } as TestRecurring | |||
test/lib/utils.ts
Outdated
| const string = rule.toString() | ||
| const rrule2 = RRule.fromString(string, rule.options.dtstart) | ||
| const str = rule.toString() | ||
| const rrule2 = RRule.fromString(str /*, rule.options.dtstart */) |
src/rrule.ts
Outdated
|
|
||
| // used by toString() | ||
| this.origOptions = {} | ||
|
|
||
| this.options = {} | ||
|
|
||
| const invalid = [] | ||
| const invalid: any[] = [] |
There was a problem hiding this comment.
Could we use unknown and some type guards instead of any here?
src/rrule.ts
Outdated
| } | ||
| } else if (/^[+-]?\d+$/.test(value)) { | ||
| value = Number(value) | ||
| } | ||
| key = key.toLowerCase() | ||
| // @ts-ignore |
There was a problem hiding this comment.
I get really intensely curious when I see @ts-ignore's, could we leave little notes about what we're avoiding with them?
There was a problem hiding this comment.
Didn't realize there were like 15 of these, maybe not a good use of time after all
| @@ -720,6 +908,7 @@ export default class RRule { | |||
| // XXX: can this ever be in the array? | |||
| // - compare the actual date instead? | |||
| if (!contains(poslist, res)) poslist.push(res) | |||
| // tslint:disable-next-line:no-empty | |||
| } catch (e) {} | |||
There was a problem hiding this comment.
oh my... i guess this is fine for now, but, oh my...
src/rrule.ts
Outdated
| @@ -1167,22 +1353,23 @@ class Iterinfo { | |||
| } | |||
| } | |||
|
|
|||
| ydayset (year, month, day) { | |||
| ydayset (_: any, __: any, ___: any) { | |||
There was a problem hiding this comment.
Can we just declare this as ydayset () { and get the same thing?
src/rrule.ts
Outdated
| return [range(this.yearlen), 0, this.yearlen] | ||
| } | ||
|
|
||
| mdayset (year, month, day) { | ||
| mdayset (_: any, month: number, __: any) { |
There was a problem hiding this comment.
Only the first two args seem useful here.
src/rrulestr.ts
Outdated
| _handle_int (rrkwargs, name, value, options) { // eslint-disable-line | ||
| export default class RRuleStr { | ||
| // tslint:disable-next-line:variable-name | ||
| _handle_DTSTART (rrkwargs: Options, name: string, value: string, _: any) { |
There was a problem hiding this comment.
We can trim off the trailing _ args on functions in this file if we want
src/rrulestr.ts
Outdated
| } | ||
|
|
||
| _handle_FREQ (rrkwargs, name, value, options) { // eslint-disable-line | ||
| _handle_FREQ (rrkwargs: Options, _: any, value: FreqKey, __: any) { | ||
| // eslint-disable-line |
There was a problem hiding this comment.
These eslint disables aren't doing anything
src/rrulestr.ts
Outdated
| let name: string | ||
| let value: string | ||
| let parts: string[] | ||
| let parms: any |
src/rrulestr.ts
Outdated
| let value: string | ||
| let parts: string[] | ||
| let parms: any | ||
| let parm: any |
src/rrulestr.ts
Outdated
|
|
||
| const invalid = [] | ||
| parse (s: string, options: Partial<RRuleStrOptions> = {}): RRule | RRuleSet { | ||
| const invalid: any[] = [] |
There was a problem hiding this comment.
Similar thing here with any vs unknown, I'm curious how hard it would be to guard our way to inferred-stringiness
src/rrulestr.ts
Outdated
|
|
||
| RRuleStr.prototype._handle_DTSTART = function (rrkwargs, name, value, options) { | ||
| rrkwargs[name.toLowerCase()] = dateutil.untilStringToDate(value) | ||
| // tslint:disable-next-line:variable-name |
There was a problem hiding this comment.
Maybe do a // tslint:disable:variable-name here and // tslint:enable:variable-name after this block of stuff
src/callbackiterresult.ts
Outdated
| const allowedMethods = ['all', 'between'] | ||
| if (!contains(allowedMethods, method)) { | ||
| throw new Error('Invalid method "' + method + | ||
| '". Only all and between works with iterator.') | ||
| throw new Error( |
There was a problem hiding this comment.
I scrapped that bit altogether. Thanks, Typescript!
src/rrule.ts
Outdated
| 52 + | ||
| pymod( | ||
| lyearlen + pymod(lyearweekday - (rr.options.wkst as number), 7), | ||
| 7 |
src/rrule.ts
Outdated
| @@ -999,14 +1166,14 @@ class Iterinfo { | |||
| const l = Math.floor(32 + 2 * e + 2 * i - h - k) % 7 | |||
| const m = Math.floor((a + 11 * h + 22 * l) / 451) | |||
| const month = Math.floor((h + l - 7 * m + 114) / 31) | |||
| const day = (h + l - 7 * m + 114) % 31 + 1 | |||
| const day = ((h + l - 7 * m + 114) % 31) + 1 | |||
There was a problem hiding this comment.
👍 Operator precedence is a little too magical sometimes, this is probably good.
|
@arolson101 @jakubroztocil @thefliik I believe this branch is now ready for consideration. Please take a look! |
|
I was holding off looking at it because of all the commits going in after you asked for review ;) |
|
@arolson101 Conflicts resolved! Unfortunately, that last merge to master didn't come with a test, so I'm trying to write one now, but that shouldn't block consideration of this PR. Thanks! |
There was a problem hiding this comment.
Nothing pops out as me as bad, and the code is much more readable to me now. This being said, I'm still not familiar with the actual logic processing of the library, and I haven't downloaded and tested this branch so I can't speak to that.
LGTM tho 👍
and Awesome Job!
|
@thefliik I have plans to break down the logic later on. It's spaghetti. @arolson101 This is ready for a look. I have 2 follow on branches after this! |
|
I don't have time to look it over, but as long as it passes the test I'm happy |
Addresses #229
Note there is a breaking change here: the types exported from
dist/es6/index.d.tsdo not conform to the structure in the originalindex.d.ts. UsingRRuleas both a namespace and a class seemed overly complicated, so I simplified it, but open to discussion.I'd say this PR probably warrants a minor version bump in the package.