Refactor internal PaginatedResult API to have non-optional methods
#2021
Conversation
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PaginatedResult
Client->>PaginatedResult: first()
alt Link exists
PaginatedResult-->>Client: Promise<PaginatedResult>
else Link missing
PaginatedResult-->>Client: throw ErrorInfo
end
Client->>PaginatedResult: next()
alt Next link exists
PaginatedResult-->>Client: Promise<PaginatedResult>
else No next link
PaginatedResult-->>Client: null
end
Client->>PaginatedResult: isLast()
PaginatedResult-->>Client: Boolean (not hasNext())
Possibly related issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (7)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
temporarily deployed
to
staging/pull/2021/bundle-report
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/common/lib/client/paginatedresource.ts (2)
150-150: Consider using distinct error codes for different error scenarios.Both error messages use the same error code (50000) and status code (500). For better error identification and troubleshooting, consider using distinct error codes for different error scenarios.
- throw new ErrorInfo('Missing link to the first page of results', 50000, 500); + throw new ErrorInfo('Missing link to the first page of results', 50001, 500); - throw new ErrorInfo('Missing link to the current page of results', 50000, 500); + throw new ErrorInfo('Missing link to the current page of results', 50002, 500);Also applies to: 158-158
147-147: Safe use of non-null assertion after existence check.The non-null assertion operator (
!) is used after checking for the existence of properties, which is safe in this context. However, consider that some linting configurations discourage its use. An alternative could be using optional chaining with a default value, though the current implementation is perfectly valid.Also applies to: 155-155, 163-163
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/common/lib/client/paginatedresource.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/common/lib/client/paginatedresource.ts (1)
src/platform/web/index.ts (1)
ErrorInfo(48-48)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test-node (20.x)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-node (18.x)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-node (16.x)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-npm-package
🔇 Additional comments (8)
src/common/lib/client/paginatedresource.ts (8)
4-4: Good addition of the ErrorInfo import.Adding the explicit import for ErrorInfo is appropriate since you're now using it in the refactored methods for standardized error handling.
137-137: Good encapsulation with private property.Adding a private property to store the relParams is a good practice for encapsulation. This allows all the navigation methods to access the pagination link parameters consistently through a class property.
145-151: Clear implementation of first() method with proper error handling.The implementation correctly checks for the existence of the first page link before attempting to navigate to it, and throws a descriptive error if the link is missing. This is more robust than the previous implementation which would result in a less specific TypeError.
153-159: Clear implementation of current() method with proper error handling.Similar to the first() method, this implementation includes appropriate checks and error handling, which improves the developer experience when debugging issues with pagination.
161-167: Well-implemented next() method with appropriate null return.The next() method correctly returns null when there is no next page, which is the expected behavior for paginated APIs and maintains compatibility with the previous implementation.
169-171: Good implementation of helper methods for checking link existence.The hasFirst(), hasCurrent(), and hasNext() methods provide a clear and consistent way to check for the existence of pagination links. Using the 'in' operator is more explicit and safer than checking for truthiness.
Also applies to: 173-175, 177-179
181-183: Simple but effective isLast() implementation.The isLast() method logically returns the negation of hasNext(), which is a clean way to determine if a page is the last one in the sequence.
145-183: Overall excellent refactoring of the PaginatedResult class.The refactoring of the PaginatedResult class successfully replaces dynamically assigned optional function properties with explicit method implementations. This:
- Aligns with the public API declarations in ably.d.ts
- Provides better TypeScript type safety
- Improves error handling with descriptive messages
- Creates a more structured and maintainable codebase
- Enables union types between internal and public PaginatedResult APIs as needed for PR #2013
This implementation achieves the goals described in the PR objectives without introducing breaking changes for library users.
| return this.get(this._relParams!.first); | ||
| } | ||
|
|
||
| throw new ErrorInfo('Missing link to the first page of results', 50000, 500); |
There was a problem hiding this comment.
I don't think it should be 500 or any 5xx, we wrap client.request response into paginated result, and server doesn't guarantee that all requests are paginated, e.g. /time is not paginated. It's better to throw 4xx instead
There was a problem hiding this comment.
Will check how client.request('/time') looks right now, it may help answer the null/error on missing page discussion below too.
There was a problem hiding this comment.
will change to "No link to the first page of results, 40400, 404"
There was a problem hiding this comment.
On a side note re res.request() method. It wraps the response in HttpPaginatedResponse class, which is by the spec #HP2 should override the next() and first() calls so they return new empty HttpPaginatedResponse instance.
This is not the case currently in ably-js, as the internal HttpPaginatedResponse simply extends the PaginatedResult and suffers from the same null/missing link problems.
It feels like the whole paginated response handling in ably-js is a bit outdated. Created for this #2023
There was a problem hiding this comment.
VeskeR
force-pushed
the
improve-paginated-result-api
branch
from
68f4c30 to
8d0b16d
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2021/bundle-report
VeskeR
force-pushed
the
improve-paginated-result-api
branch
from
8d0b16d to
be5e457
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2021/bundle-report
This changes condition based function properties in the internal `PaginatedResult` to regular class methods. The spec TG5 [1] does not specify if the link to the first page (or current) can be missing in the response and what should be done in such case, so the new implementation is the best effort attempt to bring the internal `PaginatedResult` API in line with the public API, while keeping the same function behavior as before. The new implementation will result in an thrown error if there was no link to the first or current page, just like it would before since those methods would not exist on the class. Essentially this changes only the type of the error thrown: for example, instead of "TypeError: page.current is not a function" the client will get "Error: Missing link to the current page of results". Users of the client library should not have expected the .first() and .current() methods to be optional before, as the public definition for `PaginatedResult` in ably.d.ts declares them as regular methods, thus this should not be a breaking change. This makes internal `PaginatedResult` match the public one in ably.d.ts. This enables union types between internal and public APIs for `PaginatedResult`, which will be needed for new Push device listSubscriptions API in [2]. [1] https://sdk.ably.com/builds/ably/specification/main/features/#TG5 [2] #2013
VeskeR
force-pushed
the
improve-paginated-result-api
branch
from
be5e457 to
45f51b2
Compare
This changes condition-based function properties in the internal
PaginatedResultto regular class methods.The spec #TG5 does not specify if the link to the first page (or current) can be missing in the response and what should be done in such
case, so the new implementation is the best-effort attempt to bring the internal
PaginatedResultAPI in line with the public API, while keeping the same function behavior as before.The new implementation will result in a thrown error if there was no link to the first or current page, just like it would before since those methods would not exist on the class. Essentially this changes only the type of the error thrown: for example, instead of "TypeError: page.current is not a function" the client will get "Error: Missing link to the current page of results".
Users of the client library should not have expected the
.first()and.current()methods to be optional before, as the public definition forPaginatedResultin ably.d.ts declares them as regular methods, thus this should not be a breaking change.This makes internal
PaginatedResultmatch the public one in ably.d.ts. This enables union types between internal and public APIs forPaginatedResult, which will be needed for the new Push device listSubscriptions API in #2013.Summary by CodeRabbit
Summary by CodeRabbit