feat(schedules): add sunrise() for the sunrises seen from a place on Earth in @observerly/orderly - #69
Conversation
e75d02a to
e913653
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new observer validation accepts non-numeric latitude/longitude values like undefined/null at runtime due to Number.isNaN checks, which can violate the stated “refuse invalid observers” contract.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a new sunrise(observer) schedule to the schedules module, backed by @observerly/astrometry, and wires it through the public API/build/docs so consumers can schedule daily sunrises for a location (including polar-day/night gaps and pole exhaustion).
Changes:
- Introduces
src/schedules/sun.tswithsunrise()plus exportedObservertype and day-walk logic. - Adds comprehensive Vitest coverage for day-to-day behavior, global longitude offsets, elevation effects, polar gaps, and Date-horizon behavior.
- Updates public exports, docs, changeset, and marks
@observerly/astrometryas external in the rolldown build.
File summaries
| File | Description |
|---|---|
| tests/sunrise.spec.ts | Adds functional tests for sunrise sequencing, strict “after”, global locations, elevation, purity, and input refusal. |
| tests/sunrise-horizons.spec.ts | Adds tests for polar day/night gaps, pole exhaustion, and Date min/max edge behavior (plus oracle replay vs astrometry). |
| src/schedules/sun.ts | Implements Observer + sunrise() via a bounded day-walk using getSunrise. |
| src/schedules/index.ts | Re-exports sunrise and Observer from schedules entrypoint. |
| src/index.ts | Exposes sunrise and Observer from the package root API. |
| rolldown.config.ts | Marks @observerly/astrometry as external so it isn’t bundled into dist. |
| README.md | Documents the new sunrise() schedule and adds a “The Sun” section with example usage. |
| .changeset/add-sunrise.md | Declares a minor bump for adding sunrise(observer) to the public API. |
Review details
Suppressed comments (1)
src/schedules/sun.ts:67
- Same issue as latitude: using
Number.isNaN(longitude)allowsundefinedthrough and treatsnullas 0. UsingNumber.isFinite(longitude)makes the constructor reliably reject non-numeric inputs at runtime.
if (Number.isNaN(longitude) || longitude < -180 || longitude > 180) {
throw new RangeError(`${method} requires a longitude in degrees between -180 and 180`);
}
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
e913653 to
2aafa87
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The sunrise schedule’s horizon is set two days inside the Date limit, which can prematurely exhaust and skip a valid last-day occurrence near the end of time.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Lite
2aafa87 to
cc74766
Compare
…Earth in @observerly/orderly
cc74766 to
0c0fb3f
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The feature is isolated, thoroughly tested (including edge cases), and the only feedback is a minor maintainability naming clarification.
Review details
Suppressed comments (2)
src/schedules/sun.ts:51
- This file defines a local
HORIZON_IN_MILLISECONDSwith different semantics (MAX − 2 days) than the exportedHORIZON_IN_MILLISECONDSused by other schedule walks (MAX − 1 day). Reusing the same name for a different horizon makes code search and future refactors error-prone; consider renaming this constant to clearly indicate it is the astrometry-specific horizon.
// astrometry resolves a date's event about the solar transit nearest the date's mean solar noon, which for an
// observer at the far west falls a full day past the date's midnight, and it refuses a date whose search
// leaves the range a Date can hold. Asked for the last day of that range by an observer more than 150 degrees
// west, it throws; asked for the day before, it never does. So the walk keeps two days clear of either end,
// one more than the calendar walks keep, and surrenders those days at the end of the year 275760 deliberately,
// as they do, rather than resolving the edge by a strategy of its own.
const HORIZON_IN_MILLISECONDS = MAXIMUM_INSTANT_IN_MILLISECONDS - 2 * MILLISECONDS_IN_DAY;
src/schedules/sun.ts:114
- This reference also needs to use the renamed astrometry-specific horizon constant to avoid confusion with the shared calendar horizon used elsewhere in schedules.
// A day before the horizon cannot be asked for either, but the days after it can.
if (midnight < -HORIZON_IN_MILLISECONDS) {
continue;
}
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Lite
| // Nothing past the horizon can be asked for, so nothing past it can follow. | ||
| if (midnight > HORIZON_IN_MILLISECONDS) { | ||
| return null; | ||
| } |
What This Changes
sunrise(observer)to the schedules module: the sunrises seen from a place on Earth, one a day on the days the Sun rises, resolved with@observerly/astrometryto the standard almanac convention, the upper limb of the Sun touching the horizon with refraction and the observer's elevation included. The exportedObservertype is{ latitude, longitude, elevation? }: degrees north and east positive, metres above sea level, sea level when omitted.next(after)lies strictly after the instant, is pure, and returns null only where no sunrise falls within 400 days. That is the case at the poles, where astrometry places no sunrise on any day, and can be within a tenth of a degree of them, where its day-by-day search misses the one crossing in some years. The days of a polar day or a polar night are walked past.RangeErrornamingsunrise(): a latitude outside -90 to 90, a longitude outside -180 to 180, or an elevation that is not finite. Only a finite number passes, so a coordinate that is missing or null, as data read without a type may carry, is refused rather than read as zero. astrometry answers what it cannot place with silence, which would otherwise be a schedule that never fires and never says why.externalin the rolldown build, sodist/index.jsimports@observerly/astrometry/sunrather than bundling a copy of it.Notes For Reviewers
candidates.tsmakes for the calendar walks, and a test pins it: an observer at 180 degrees west, asked from a day and a millisecond before the end of time, exhausts rather than throws. With a one-day margin that test throwsInvalid time value.previewandtake: 2rather than a singlenext(), because for an observer whose sunrise sits near midnight UTC a UTC day holds two sunrises once or twice a year, and a singlenext()from a daily cron would drop one. It bounds the schedule withbetweento the day a queue can hold a message back, so a polar night yields nothing rather than the RangeErrorsend()raises for an instant out of reach, and it includes the Workflow class the wrangler config names, so it deploys as written. The whole example was typechecked against@cloudflare/workers-typesand orderly's source.pnpm testnow prints seven Vite notices that astrometry's shipped sourcemaps point at source files it does not publish. They are harmless; the fix belongs upstream in astrometry.prefer-readonly-parameter-typeswarnings insun.ts, the kind every schedule module already carries, the contract's own Date parameter included. No other warning is new.Checklist