Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant FetchOp as Fetch\nOperation
participant Scope as Effection\nScope
participant Native as Native\nFetch API
participant Server as HTTP\nServer
App->>FetchOp: yield* fetch(url, init?)
FetchOp->>Scope: obtain scope AbortSignal
FetchOp->>FetchOp: merge init.signal with scope.signal
FetchOp->>Native: native fetch(url, { signal: merged })
Native->>Server: HTTP request
Server-->>Native: HTTP response
Native-->>FetchOp: Response
FetchOp-->>App: returns FetchResponse facade
App->>FetchOp: yield* response.json()/text()/body()/expect()
FetchOp->>FetchOp: check bodyUsed / consumed guard
FetchOp->>Native: read/stream body with AbortSignal
Native-->>FetchOp: chunks / parsed value
FetchOp-->>App: Operation result or thrown HttpError
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Comment |
commit: |
- fetch().json(), fetch().text(), fetch().body() for single yield* - fetch().expect().json() for validation before consumption - Rename ensureOk() to expect() - Remove clone() method - Update tests and README with fluent API examples
cowboyd
left a comment
cowboyd
left a comment
There was a problem hiding this comment.
Should this be a resource?
Also, we should have documentation on the individual symbols
fetch/fetch.test.ts
Outdated
| }); | ||
| }); | ||
|
|
||
| function* captureError( |
There was a problem hiding this comment.
Done - now using box() with Effection's Ok/Err/Result types for error testing.
fetch/fetch.ts
Outdated
| let signal = init?.signal | ||
| ? AbortSignal.any([init.signal, scopeSignal]) | ||
| : scopeSignal; |
There was a problem hiding this comment.
Should we even allow an ignore any signal passed in? Even if we don't, we should decide that before we create an unnecessary abort signal.
There was a problem hiding this comment.
just removed it and removed signal from options type.
fetch/fetch.ts
Outdated
| let consumed = false; | ||
|
|
||
| let guardBody = () => { | ||
| if (consumed || response.bodyUsed) { | ||
| throw new Error("Body has already been consumed"); | ||
| } | ||
| consumed = true; | ||
| }; |
There was a problem hiding this comment.
Why are we tracking consumption state ourselves and then adding an error?
There was a problem hiding this comment.
Removed - now letting the native Response handle body consumption tracking. It throws its own error if you try to consume twice. Also removed the corresponding test.
fetch/fetch.ts
Outdated
| *ensureOk(): Operation<FetchResponse> { | ||
| if (!response.ok) { | ||
| throw new HttpError( | ||
| response.status, | ||
| response.statusText, | ||
| response.url, | ||
| self, | ||
| ); | ||
| } | ||
| return self; | ||
| }, |
There was a problem hiding this comment.
Let's keep the api as 1:1 as we can. I could see adding this as a standalone function that would accept a response, but even so, it does not need to be an operation
There was a problem hiding this comment.
Kept as Operation per user preference. Renamed from ensureOk() to expect() for fluent API brevity: fetch().expect().json(). The Operation wrapper is minimal - it just checks response.ok and throws HttpError if false.
…lation
- Add FetchInit type (Omit<RequestInit, 'signal'> & { signal?: never })
- Remove signal merging logic, always use Effection scope signal
- Remove signal-related test
- Update README to clarify cancellation is via structured concurrency
- Replace captureError() with box() for error testing - Remove 'prevents consuming body twice' test (let native Response handle it) - Add comprehensive JSDoc documentation to all exports - Remove manual consumed tracking and guardBody() - Remove signal from options (use Effection scope signal only)
|
Addressed all feedback in the latest commit:
Also removed the "prevents consuming body twice" test since the native Response handles that now. |
|
Regarding "Should this be a resource?":
A Resource would make sense if we were managing something like a connection pool or persistent WebSocket, but for individual fetch requests, Operation is the right abstraction. |
I'm not sure what this means. A resource is an operation. |
|
Sorry about that. It was AI slop slop I didn't see that it left this comment. It seems like it should be a resource, but there is no obvious lifecycle that's not covered by the abort signal. It might be more straightforward once we have the inspector so we can actually see what it looks like there. I'm planning to change this to be contextapi so I don't know if any of those factors impact this decision. |
Yeah, you're right. really the abort signal is the resource. The only reason really that it should be its own resource is to give it a name in order to contextualize the abort signal in the inspector:
This is a great idea. Also, thinking that this should make its way into Effection core at some point. |
2 participants
Motivation
Make it easier for people to use fetch with Effection by providing an Effection-native HTTP client with:
Stream<Uint8Array, void>yield* fetch(...).json()Approach
@effectionx/fetchpackage withfetch()returning a chainableFetchOperationyield* fetch(...).json()) and traditional (yield* (yield* fetch(...)).json()) APIsuseAbortSignal()for scope-aware cancellation, merged with user-provided signals viaAbortSignal.any()stream()to convertReadableStreamtoStream<Uint8Array, void>for body streamingexpect()method that throwsHttpErroron non-2xx responsesjson(parse)overload for runtime validation during parsingpnpm-workspace.yaml,tsconfig.json)fx/README.mdpointing to this package for streaming use casesSummary by CodeRabbit
New Features
Documentation
Tests
Chores