Fix "time by azimuth" crash: Cannot read properties of undefined (reading 'latitude') - #565
Open
b3nj1 wants to merge 2 commits into
Open
Fix "time by azimuth" crash: Cannot read properties of undefined (reading 'latitude')#565b3nj1 wants to merge 2 commits into
b3nj1 wants to merge 2 commits into
Conversation
…ding 'latitude') _getSunTimeByAzimuth() took a `degree` parameter ahead of `tprop`, but its only caller (getPropValue()'s pdsTimeByAzimuth case) never passed it, so `tprop` was actually undefined inside the function and any access to `tprop.latitude` threw. This broke every "time by azimuth" use of the time-inject node (main schedule and node-editor preview) as soon as a stable 3.0.x release reached npm. Drop the unused `degree` parameter and derive it from `this.angleType`, matching the equivalent _getSunTimeByElevation() implementation. Also fix dateTimeHelper.getDateOfText() to return a Date it is given as-is instead of round-tripping it through String()/getTimeOfText(), which silently discarded the date portion and only kept the time of day. This surfaced once the crash above was fixed: pdsTimeByAzimuth (and pdsTimeByElevation, which shares the same code path) return a Date object from getPropValue(), and getTimeProp()'s generic branch passes that Date through getDateOfText(). Fixes rdmtc#561, rdmtc#557. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UJCVNfqxyRfmyJZKAVWjCG
) getTimeProp() has no dedicated branch for pdsTimeByElevation, pdsTimeByElevationRise/Set/Next, so they fell through to the generic branch, which passes whatever getPropValue() returns into getDateOfText(). getPropValue() returns an ISunTimeSingle object ({value: Date, name, ts, ...}) for the Rise/Set/Next variants, and getDateOfText() stringifies non-Date objects before parsing them, turning it into "[object Object]" and throwing - reproducing rdmtc#466 ("Error using inject with 'next rise time by elevation': could not evaluate pdsTimeByElevationRise"). Unwrap the Date from such result objects before falling back to getDateOfText(), the same way the existing pdsTime/pdmTime branches already do via Object.assign(result, ...). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UJCVNfqxyRfmyJZKAVWjCG
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #561 and #557 — every use of "time by azimuth" (the
time-injectnode's schedule type, and the node editor's live preview for that field) throws:While testing this, found and fixed the same class of bug for "time by elevation" (#466).
Root cause 1:
_getSunTimeByAzimuthcrash (#561, #557)_getSunTimeByAzimuth(dNow, azimuthAngle, degree, tprop, latitude, longitude)takes adegreeparameter ahead oftprop, but its only caller (getPropValue()'spdsTimeByAzimuthcase) calls it asthis._getSunTimeByAzimuth(dNow, parseFloat(data.value), data)— 3 arguments. That leavestpropundefinedinside the function, sotprop.latitudethrows aTypeError. This has apparently been present sincedegreewas added back in 2022, but only reached users once 3.0.1 became the first non-alpha/beta release on npm (2026-06-24), which lines up with reports of this breaking around end of June.The fix drops the unused
degreeparameter and derives it fromthis.angleType, the same way the sibling_getSunTimeByElevation()already does — no caller needs to change.Root cause 2:
getDateOfTextmanglesDateinput (surfaced by fixing #1)Once the crash above is fixed,
getTimeProp()'s generic fallback branch (used bypdsTimeByAzimuth, which has no dedicated branch there) still produced a wrong date: it passes theDateobject returned bygetPropValue()intodateTimeHelper.getDateOfText(), which round-trips any object input throughString()and re-parses it — silently discarding the date portion and keeping only the time of day.getDateOfText()now returns aDateit's given as-is instead of re-stringifying it.Root cause 3:
getTimePropgeneric branch mishandlespdsTimeByElevation*results (#466)getTimeProp()has no dedicated branch forpdsTimeByElevation/pdsTimeByElevationRise/pdsTimeByElevationSet/pdsTimeByElevationNexteither, so they also fall through to the generic branch. Unlike azimuth,getPropValue()returns a fullISunTimeSingleobject ({value: Date, name, ts, ...}) for the Rise/Set/Next variants — passing that intogetDateOfText()stringifies it to"[object Object]"and throws, reproducing #466 ("Error using inject with 'next rise time by elevation': could not evaluate pdsTimeByElevationRise"). The generic branch now unwrapsres.valuewhen it's already a validDate, before falling back togetDateOfText().Test plan
test/20_time-inject_spec.js:getPropValue()withtype: 'pdsTimeByAzimuth'exactly as the crash's stack trace shows, and asserts a correctDateis returned instead of throwing.getTimeProp()(the method that actually throws per Error using inject with "next rise time by elevation": could not evaluate pdsTimeByElevationRise #466) for each variant and asserts a validDateis returned instead of throwing.npm run testnode— 117 passing vs. 113 onmaster(4 tests added by this PR), same 10 pre-existing failures asmaster(timezone-environment-dependent, unrelated to this change).npm run lint— no new warnings/errors.🤖 Generated with Claude Code
https://claude.ai/code/session_01UJCVNfqxyRfmyJZKAVWjCG