Skip to content

refactor: large initial factor for big fixes and performance - #2

Merged
shavonn merged 1 commit into
mainfrom
refactor/initial-refactor
Feb 21, 2026
Merged

shavonn merged 1 commit into
mainfrom
refactor/initial-refactor

Conversation

@shavonn

@shavonn shavonn commented Feb 21, 2026

Copy link
Copy Markdown

Refactored the core recurrence-rule implementation to share query/caching logic across RRule and RRuleSet, while also fixing a few correctness/performance issues in iteration and caching behavior.

Bug Fixes

  • Fixed removeFilteredDays accumulation bug

    • Was overwriting filtered on each iteration instead of accumulating. Only the last day's filter status was returned, causing sub-daily frequencies to
      potentially skip the wrong number of days when earlier days in the set were filtered out.
  • Fixed RRuleSet.clone() inverted noCache

    • Changed new RRuleSet(!!this._cache) to new RRuleSet(!this._cache). The constructor param is noCache, so the old code was enabling cache on uncached
      sets and vice versa.
  • Removed dead lno1wkst = 0 assignment

    • The value was assigned inside an if block but never read afterward. Changed declaration from let to const.

Architecture

  • Extracted RRuleBase abstract class
    • Moved shared query methods (all, between, before, after, count) and caching logic into RRuleBase.
    • RRule extends RRuleBase — keeps options, _iter, toString, toText, NLP methods.
    • RRuleSet extends RRuleBase — no longer extends RRule or calls super({}, noCache) with fake empty options.
    • Exported RRuleBase from src/index.ts so consumers can use instanceof RRuleBase for any rule-like object.
    • Updated test utilities to use RRuleBase for shared type checking.

Performance

  • rrules()/exrules() — eliminated serialize+reparse

    • Was: this._rrule.map((e) => rrulestr(e.toString())) — serializes to RFC string then re-parses.
    • Now: this._rrule.map((e) => e.clone()) — direct clone, removed the rrulestr import entirely.
  • _addRule/_addDate — eliminated O(n) array allocation

    • Was: collection.map(String).includes(String(rrule)) — allocates a new array on every call.
    • Now: collection.some((r) => String(r) === str) — short-circuits without allocating.
    • Same pattern for _addDate: .some((d) => d.getTime() === time) instead of .map(Number).includes(Number(date)).
  • Added cache size bounds (src/cache.ts)

    • Added MAX_CACHE_SIZE = 64 constant.
    • before/after/between caches now evict the oldest entry (FIFO) when the limit is reached, preventing unbounded memory growth on long-lived instances.

Type Safety

  • Narrowed bynweekday to tuple type (src/types.ts)

    • Changed bynweekday: number[][] | null to bynweekday: [number, number][] | null in ParsedOptions, enabling safe destructuring without ! assertions in
      monthinfo.ts and parseoptions.ts.
  • Narrowed toText options to ParsedOptions (src/nlp/totext.ts)

    • Changed private options: Partial to private options: ParsedOptions, removing ~40 ! assertions on this.options.freq, this.options.interval,
      this.options.bymonthday, etc.
  • Added explicit return types to dateutil helpers (src/dateutil.ts)

    • getWeekday(): number, getMonthDays(): number, monthRange(): [number, number].
    • Eliminated downstream ! assertions in datetime.ts and yearinfo.ts where these return values were used.
  • Added explicit return type to extractName (src/rrulestr.ts)

    • Typed as { name: string; value: string } with default destructuring values, removing ! assertions on name and value.
  • Added default destructuring values in parseString (src/parsestring.ts)

    • const [key = '', value = ''] = attr.split('=') and similar patterns, eliminating undefined-possibility assertions.
  • Fixed Cache.all type (src/cache.ts)

    • Was: Date[] | Partial | false — Partial was never stored as all.
    • Now: Date[] | false.
  • Clarified cache miss check (src/cache.ts)

    • Changed if (!cached && this.all) to if (cached === false && this.all) — makes intent explicit for "not in cache" vs "cached as null".

Code Cleanup

Converted C-style loops to for...of across 7 files

  • monthinfo.ts, optionstostring.ts, parseoptions.ts, yearinfo.ts, iterset.ts, iter/index.ts, iter/poslist.ts — eliminates uncertain array-index !
    assertions.

Removed dead code (src/iter/index.ts)

  • Deleted redundant interval === 0 check (already checked at function entry).

Removed redundant instanceof runtime guards (src/rruleset.ts)

  • _addRule had if (!(rrule instanceof RRule)) and _addDate had if (!(date instanceof Date)) — parameters are already typed, so these were always true.
    Removed guards and corresponding tests.

Fixed test parse() null safety (test/lib/utils.ts)

  • Added null guard for regex match result before destructuring. Simplified with Number() instead of intermediate variables.

Features

  • toText() now includes start date (src/nlp/totext.ts)
    • When dtstart is explicitly set in rule options, toText() appends , starting [formatted date] to the output.
    • Example: "every 2 months on the 15th, starting February 18, 2026".
    • Rules without an explicit dtstart are unaffected.

Tooling

  • Updated README (README.md)
    • Fixed stale badges, broken code examples, byeaster docs, wkst typo, LICENCE/LICENSE link, dev tooling (Biome), and documented new toText() starting
      date behavior.

Copilot AI review requested due to automatic review settings February 21, 2026 03:46
@shavonn
shavonn merged commit 2a0bd87 into main Feb 21, 2026
5 checks passed
@shavonn
shavonn deleted the refactor/initial-refactor branch February 21, 2026 03:49

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR modernizes the build/tooling setup and refactors the recurrence querying/caching architecture so both RRule and RRuleSet share the same query methods, while also tightening types and addressing correctness/performance issues in iteration and caching.

Changes:

  • Introduces RRuleBase to centralize query methods (all/between/before/after/count) and caching behavior, and updates RRule/RRuleSet accordingly.
  • Updates iteration/filtering, helpers, and cache behavior (including adding cache size bounds) plus broad type-safety/loop refactors across core modules.
  • Migrates project tooling from webpack/mocha/eslint/prettier to Vite + Biome + Changesets + Bun, and updates CI workflows and package distribution metadata.

Reviewed changes

Copilot reviewed 66 out of 72 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
webpack.config.js Removes legacy webpack ES5 build configuration.
vite.config.ts Adds Vite library build + unplugin-dts types bundling.
tsconfig.json Updates TS compiler settings for modern ESM/bundler workflow.
tsconfig.build.json Removes old build-specific TS config used by webpack pipeline.
test/rrulestr.test.ts Reformats/updates tests for new exports and style.
test/parsestring.test.ts Updates imports and formatting for new module entrypoints.
test/parseoptions.test.ts Updates imports/formatting; keeps TZID/byweekday coverage.
test/optionstostring.test.ts Updates imports/formatting; keeps serialization tests.
test/nlp.test.ts Updates NLP tests to reflect toText() “starting …” behavior.
test/mocha.opts Removes Mocha config (project now uses Jest).
test/lib/utils.ts Updates shared test helpers to use RRuleBase and improves parsing safety.
test/helpers.test.ts Removes tests for deleted helper APIs and modernizes imports.
test/datewithzone.test.ts Reformats/updates tests for new tooling/style.
test/dateutil.test.ts Minor formatting updates.
test/cache.test.ts Updates types/import style; keeps cache behavior tests.
src/weekday.ts Formatting + stricter indexing types in toString().
src/types.ts Tightens types (notably bynweekday) and updates type-only imports.
src/rrulestr.ts Refactors parsing/initialization; removes helper includes usage.
src/rruleset.ts Refactors to extend RRuleBase, optimizes cloning, and list getters.
src/rrulebase.ts New shared base class for query methods + caching.
src/rrule.ts Refactors to extend RRuleBase and delegates iteration to iter().
src/parsestring.ts Refactors parsing with safer destructuring/defaults.
src/parseoptions.ts Refactors loops, removes helper includes, tightens byweekday handling.
src/optionstostring.ts Refactors to avoid helper includes; modernizes serialization.
src/nlp/parsetext.ts Refactors NLP parser code style/types.
src/nlp/index.ts Updates NLP exports and header metadata; refactors to arrow functions.
src/nlp/i18n.ts Formatting/type cleanups.
src/masks.ts Formatting and minor refactors.
src/iterset.ts Refactors iteration set logic imports/types and removes helper includes.
src/iterresult.ts Tightens types and refactors control flow in iter result handling.
src/iterinfo/yearinfo.ts Loop refactors and removes helper includes usage.
src/iterinfo/monthinfo.ts Uses tuple-typed bynweekday and refactors loops.
src/iterinfo/index.ts Refactors iteration info orchestration and typing.
src/iterinfo/easter.ts Formatting-only refactor.
src/iter/poslist.ts Refactors loops and removes helper includes usage.
src/iter/index.ts Fixes filtered-day accumulation and refactors iteration logic.
src/index.ts Updates public exports, including new RRuleBase.
src/helpers.ts Removes includes, clone, and padStart; modernizes helpers.
src/datewithzone.ts Refactors to use Number.isNaN and tightens types.
src/dateutil.ts Refactors and replaces custom padStart with String().padStart.
src/datetime.ts Refactors to remove helper includes usage and modernize style.
src/callbackiterresult.ts Refactors types/style for callback-based iteration.
src/cache.ts Adds bounded cache sizing and clarifies cache-miss logic.
package.json Renames package scope, switches to ESM exports map, updates tooling deps/scripts.
jest.config.ts Adds typed Jest config in TS.
jest.config.js Removes old CommonJS Jest config.
index.html Adds Vite dev entry HTML.
codecov.yml Removes Codecov config (legacy nyc/codecov pipeline removed).
bower.json Removes legacy Bower metadata.
biome.json Adds Biome formatter/linter configuration.
LICENSE Updates copyright lines and fixes “LICENCE/LICENSE” wording.
.vscode/launch.json Removes old Mocha debug configuration.
.prettierrc.json Removes Prettier config (Biome now used).
.prettierignore Removes Prettier ignore (Biome/gitignore now used).
.npmrc Adds GitHub Packages registry configuration for scoped publishing.
.husky/pre-commit Updates pre-commit hook command (now Bun-based).
.gitignore Updates ignores for Vite/Bun/Biome-era outputs and editor files.
.github/workflows/test.yml Adds Bun-based CI workflow matrix.
.github/workflows/release.yml Adds Changesets-based release workflow for GitHub Packages.
.github/workflows/nodejs.yml Removes legacy Node CI workflow.
.github/FUNDING.yml Updates funding recipients list.
.eslintrc.js Removes ESLint configuration (Biome now used).
.eslintignore Removes ESLint ignore file (Biome now used).
.changeset/loose-seas-share.md Adds Changeset documenting the refactor/bugfix/tooling changes.
.changeset/config.json Adds Changesets configuration.
.changeset/README.md Adds Changesets README.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/rruleset.ts
Comment on lines 201 to 215
function _addRule(rrule: RRule, collection: RRule[]) {
if (!(rrule instanceof RRule)) {
throw new TypeError(String(rrule) + ' is not RRule instance')
throw new TypeError(`${String(rrule)} is not an instance of RRule`);
}

if (!includes(collection.map(String), String(rrule))) {
collection.push(rrule)
const str = String(rrule);
if (!collection.some((r) => String(r) === str)) {
collection.push(rrule);
}
}

function _addDate(date: Date, collection: Date[]) {
if (!(date instanceof Date)) {
throw new TypeError(String(date) + ' is not Date instance')
throw new TypeError(`${String(date)} is not an instance of Date`);
}

Copilot AI Feb 21, 2026

Copy link

Choose a reason for hiding this comment

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

PR description says the redundant instanceof runtime guards in _addRule/_addDate were removed, but they are still present here (only the error message changed). Either update the PR description to match the implementation, or remove the guards (and adjust tests) if that was the intent.

Copilot uses AI. Check for mistakes.
Comment on lines +31 to +42
- name: Capture New Release Version
run: |
bun changeset status --output=release.json
echo "NEW_VERSION=$(jq -r '.releases | map(.name+"@"+.newVersion) | join(",")' release.json)" >> "$GITHUB_ENV"
rm release.json

- name: Create Release Pull Request or Publish to GitHub Packages
id: changesets
uses: changesets/action@v1
with:
publish: bun publish-package
title: "Release version ${{env.NEW_VERSION}}"

Copilot AI Feb 21, 2026

Copy link

Choose a reason for hiding this comment

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

The release workflow uses bun changeset ... and publish: bun publish-package. Bun won't execute package binaries/scripts with that syntax reliably. Use bunx changeset ... (or bun run change -- status ...) and set publish to bun run publish-package so the Changesets action can actually run the commands.

Copilot uses AI. Check for mistakes.
Comment thread src/cache.ts
this[what].push(args as IterArgs)
args!._value = value;
// Evict oldest entry if cache is full
if (this[what].length > MAX_CACHE_SIZE) {

Copilot AI Feb 21, 2026

Copy link

Choose a reason for hiding this comment

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

Cache eviction is off by one: with the current check (this[what].length > MAX_CACHE_SIZE) the cache will grow to MAX_CACHE_SIZE+1 before shifting. Change the condition to evict when the size is at/above the limit (e.g. >=) and evict before/after pushing consistently so the cache never exceeds MAX_CACHE_SIZE.

Suggested change
if (this[what].length > MAX_CACHE_SIZE) {
if (this[what].length >= MAX_CACHE_SIZE) {

Copilot uses AI. Check for mistakes.
Comment thread vite.config.ts
import dts from 'unplugin-dts/vite';
import { defineConfig } from 'vite';

const __dirname = import.meta.dirname;

Copilot AI Feb 21, 2026

Copy link

Choose a reason for hiding this comment

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

import.meta.dirname is not available in Node.js (the runtime used to load Vite config), so vite build will throw when evaluating the config. Use a Node-compatible __dirname replacement (e.g. derive from import.meta.url via fileURLToPath/dirname).

Copilot uses AI. Check for mistakes.
Comment thread .husky/pre-commit
. "$(dirname -- "$0")/_/husky.sh"

yarn lint-staged No newline at end of file
bun lint-staged

Copilot AI Feb 21, 2026

Copy link

Choose a reason for hiding this comment

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

The Husky hook runs bun lint-staged, but lint-staged is no longer listed in devDependencies nor provided as an npm script. This will fail on commit. Either add lint-staged back (and a corresponding script) or update the hook to run the intended Biome command(s) directly (e.g. bun run lint / bun run check).

Suggested change
bun lint-staged
bun run lint

Copilot uses AI. Check for mistakes.
Comment on lines +39 to +51
run: LANG=en_CA TZ=America/Vancouver bun test

- name: Run Unit Tests - US
run: LANG=en_US TZ=America/Los_Angeles bun test

- name: Run Unit Tests - Kenya
run: LANG=sw_KE TZ=Africa/Nairobi bun test

- name: Run Unit Tests - Christmas
run: LANG=en_KI TZ=Pacific/Kiritimati bun test

- name: Run Unit Tests - Japan
run: LANG=jp_JP TZ=Asia/Tokyo bun test

Copilot AI Feb 21, 2026

Copy link

Choose a reason for hiding this comment

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

The workflow runs bun test, which invokes Bun's built-in test runner rather than the package.json Jest script. Since this repo's tests are Jest ("test": "jest **/*.test.ts"), CI should run bun run test (or bunx jest ...) instead of bun test.

Suggested change
run: LANG=en_CA TZ=America/Vancouver bun test
- name: Run Unit Tests - US
run: LANG=en_US TZ=America/Los_Angeles bun test
- name: Run Unit Tests - Kenya
run: LANG=sw_KE TZ=Africa/Nairobi bun test
- name: Run Unit Tests - Christmas
run: LANG=en_KI TZ=Pacific/Kiritimati bun test
- name: Run Unit Tests - Japan
run: LANG=jp_JP TZ=Asia/Tokyo bun test
run: LANG=en_CA TZ=America/Vancouver bun run test
- name: Run Unit Tests - US
run: LANG=en_US TZ=America/Los_Angeles bun run test
- name: Run Unit Tests - Kenya
run: LANG=sw_KE TZ=Africa/Nairobi bun run test
- name: Run Unit Tests - Christmas
run: LANG=en_KI TZ=Pacific/Kiritimati bun run test
- name: Run Unit Tests - Japan
run: LANG=jp_JP TZ=Asia/Tokyo bun run test

Copilot uses AI. Check for mistakes.
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.

2 participants