-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
refactor(example): remove todo comment and normalize network errors #13755
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
9a5b92f to
93709f7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please update the example to show the usage of the new parameter timeout? This is an example, so we need to show the usage of things that we write
501838b to
f234636
Compare
|
@ematipico that highlights the new /********************************************************************
* example.ts – showing the new `timeout` option in action
*******************************************************************/
// Assume getJSON + helpers are imported from your library:
//
// import { getProducts, getProduct, addToUserCart } from './api';
async function demo(req: Request) {
try {
// 1) Short-fuse fetch: if your catalog service is slow, fail fast.
const products = await getProducts(req, { timeout: 3_000 });
console.log('products loaded in ≤3 s:', products.length);
// 2) Normal fetch – keep the default 10 s timeout.
const game = await getProduct(req, 42);
console.log('single product:', game);
// 3) POST helper also accepts timeout (and signal, base, …).
await addToUserCart(42, 'Space Adventure', { timeout: 5_000 });
console.log('cart updated!');
} catch (err) {
console.error('Request aborted or failed:', err);
}
}
This shows exactly how consumers pass the new option while keeping the helper’s surface minimal: just an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove the changesets? Changesets are only for modifications of packages, not examples
7c08617 to
f234636
Compare
Sure, updated it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for removing one changeset! There's another one to remove too
examples/ssr/src/api.ts
Outdated
| incomingReq: Request, | ||
| endpoint: string, | ||
| cb: (response: Response) => Promise<T> | ||
| timeout = 10_000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the timeout isn't used, i think it could be worth removing it altogether from the example. We try to keep examples minimal and as a starting point. So that probably means removing the abort controller too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, Just a moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@florian-lefebvre
I'm done, just keep simple. is it still Okay?
58950e1 to
e4e21ac
Compare
e4e21ac to
901ba85
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
…ithastro#13755) Co-authored-by: jp-knj <[email protected]> Co-authored-by: Florian Lefebvre <[email protected]>
Changes
remove todo comment
Renamed helper
get<T>→getJson<T>to clarify that the helper always returns parsed JSON.Timeout & abort support
Adds an
AbortControllerwith a 10 s default timeout to prevent hanging requests.Richer error messages
GET /api … → 503) from network/abort errors (failed: The user aborted a request).Less boilerplate
Helper now returns
response.json()directly, so the four public API functions no longer need their own inline callbacks.No functional change to callers
Signatures of
getProducts(),getProduct(),getUser(), andgetCart()remain identical; only internal implementation is updated.Changeset included
pnpm exec changesetthis is purely internal refactor. not add.Testing
pnpm test— all existing tests pass./productspage – data still renders.addToUserCart()still posts successfully.No new automated tests added because the helper is already covered by higher-level API tests; timeout behaviour is best validated manually.
Docs
No public-facing API surface changed, so docs update not required.
/cc @withastro/maintainers-docs for visibility, but I believe no docs edits are needed.