Document loadedFromSource cache disposition on the request context#563
Merged
Conversation
Companion to HarperFast/harper#1575 (fixes HarperFast/harper#1571): context.loadedFromSource is now assigned on caching-table gets. Adds an "Observing cache disposition" section under sourcedFrom with the exact settled semantics (staleIfError=true, stale-while-revalidate=false, dedupe-join=false, last get wins) and lists the property in both getContext() Context references. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-563 This preview will update automatically when you push new commits. |
Gemini review: get() returns a plain RecordObject, not a resource instance, so the method is not reachable where developers would want it (and the base implementation is an unoverridden stub). Direct readers to the context / RequestTarget instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nd version-gate the Context entries Codex review: staleIfError fallbacks serve stale cached data on a failed source fetch, so "fetched from source" overclaimed freshness; and the Context list entries needed the 5.1.16+ availability caveat. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-563 This preview will update automatically when you push new commits. |
Companion to HarperFast/harper#1577 (fixes HarperFast/harper#1576): the method now returns the recorded cache disposition on caching-table resource instances instead of always undefined. Corrects its reference section and restores the instance-method mention in Observing cache disposition. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-563 This preview will update automatically when you push new commits. |
…edFromSource (#1576) wasLoadedFromSource() is an unoverridden stub that always returns undefined; we've standardized on context.loadedFromSource / target.loadedFromSource as the way to observe cache disposition (harper#1576). Remove the reference section and the cross-reference from the caching-disposition note. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-563 This preview will update automatically when you push new commits. |
kriszyp
added a commit
to HarperFast/harper
that referenced
this pull request
Jul 3, 2026
wasLoadedFromSource() was declared on ResourceInterface and documented as reporting cache disposition, but its only implementation was the base-class stub that always returned undefined — no subclass ever overrode it, and get() returns a plain RecordObject with no instance methods anyway. We've standardized on context.loadedFromSource / target.loadedFromSource (this PR) as the supported way to observe cache disposition, so remove the misleading interface declaration and stub. Non-breaking: the method never returned a meaningful value. The only in-repo callers (OCSP/CRL cert verification) invoke it via `(x as any).?.()` on a plain record, so they were already reading undefined and are unaffected. Docs: HarperFast/documentation#563. Closes #1576. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
kriszyp
approved these changes
Jul 3, 2026
kriszyp
left a comment
kriszyp
left a comment
Member
There was a problem hiding this comment.
Approving — verified line-for-line accurate against harper#1575 / #1577's actual semantics (loadedFromSource / wasLoadedFromSource()). Since it pairs with those core PRs (v5.1-targeted), just land it alongside them and confirm the version marker. Thanks @kylebernhardy!
— 🤖 KrAIs (Kris's review assistant)
kriszyp
added a commit
to HarperFast/harper
that referenced
this pull request
Jul 3, 2026
…tables (#1575) * Mirror loadedFromSource onto the request context so it is observable with plain-id gets (#1571) The cache-disposition flag was only set on the RequestTarget of the get; with a plain string id the static dispatch mints an internal target the caller never sees, so Context.loadedFromSource (declared but never assigned) always read undefined. Now every site that sets the flag on the target mirrors it onto the context via a shared helper. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Set cache disposition on cache hits in the loadAsInstance=false get path and ensureLoaded; document exact stale semantics Review findings (Gemini): the loadAsInstance=false get path and ensureLoaded() set the flag on source fetch but never wrote false on cache hits, leaving a stale true on shared contexts; and with allowStaleWhileRevalidate the flag reads false (the cache-hit overwrite wins over the synchronous true at fetch start), while staleIfError fallbacks read true — jsdoc now states both. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Guard primitive targets in setLoadedFromSource Instance-API gets on loadAsInstance=false tables can pass a primitive id as the target; assigning a property on a primitive throws in strict mode (Codex review). Context mirroring still applies. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * refactor(resources): remove dead wasLoadedFromSource() stub (#1576) wasLoadedFromSource() was declared on ResourceInterface and documented as reporting cache disposition, but its only implementation was the base-class stub that always returned undefined — no subclass ever overrode it, and get() returns a plain RecordObject with no instance methods anyway. We've standardized on context.loadedFromSource / target.loadedFromSource (this PR) as the supported way to observe cache disposition, so remove the misleading interface declaration and stub. Non-breaking: the method never returned a meaningful value. The only in-repo callers (OCSP/CRL cert verification) invoke it via `(x as any).?.()` on a plain record, so they were already reading undefined and are unaffected. Docs: HarperFast/documentation#563. Closes #1576. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Kyle Bernhardy <kyle.bernhardy@gmail.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Kris Zyp <kriszyp@gmail.com>
Member
Author
|
harper#1575 has merged into |
🧹 Preview CleanupThe preview deployment for this PR has been removed. |
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
3 participants
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.
Companion docs for HarperFast/harper#1575 (fixes HarperFast/harper#1571), which makes cache disposition observable on caching-table gets via
context.loadedFromSource/target.loadedFromSource.Changes
sourcedFrom()inreference/resources/resource-api.md, documenting the settled semantics verified in the fix:truewhen the get went to the source (includingstaleIfErrorfallbacks to stale data);falsefor cache hits,onlyIfCached, stale-while-revalidate responses, and dedupe-join waits (with the note that such a "hit" can still take as long as the upstream fetch); last get in a context wins.loadedFromSourcelisted in bothgetContext(): Contextproperty references (version-gated "5.1.16+"), cross-linked to the new subsection.wasLoadedFromSource()is not implemented and its reference section is removed (commit 0862bda by @kriszyp) — the context/target API is the single supported way to observe disposition.Note for review
The text says "5.1.16" on the assumption harper#1575 ships in the next 5.1 patch. Hold merging until harper#1575 lands and confirm the actual patch version then.
Generated by an LLM (Claude Fable 5), cross-reviewed by Codex and Gemini; all findings addressed. Includes a commit from @kriszyp.