-
Notifications
You must be signed in to change notification settings - Fork 12
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
Add service for the PeerTube API, related to #10 #33
Conversation
81d6083
to
d73ea51
Compare
In the file OwnTube.tv/lib/peertubeVideosApi.test.ts : Mocking the API calls: The tests currently focus on successful responses. Consider adding tests that handle error scenarios like network issues or invalid responses from the API. Edge Cases: Refactoring for Readability: |
in the file OwnTube.tv/lib/peertubeVideosApi.ts Code Duplication: // Request interceptor // Response interceptor instance.interceptors.request.use((config) => { instance.interceptors.response.use((response) => { this.instance = instance; Using function parameters: You can pass parameters such as debugLogging as function arguments instead of using class properties. This makes the function more flexible. For example: instance.interceptors.request.use((config) => { instance.interceptors.response.use((response) => { this.instance = instance; Extract Constants: You are using some values, such as this.maxChunkSize, multiple times. Extract them into constants to make the code more readable." Error Handling: Instead of using try/catch inside the loop, you can move this logic outside the loop. This way, you'll avoid unnecessary error catching |
553fc78
to
1381893
Compare
This service will be used to interact with the PeerTube backend API. It will be used to reliably retrieve videos when needed; at the moment, its functionality is limited to ... 1. restricting the subset of videos returned to those classified as "local", "non-live", and "Safe-For-Work" 2. limiting the number of videos returned to the requested number of videos, and 3. handling pagination transparently (as the API only returns a maximum of 100 videos on each request) In the future, this service will be extended to include more functionality, such as ... 1. handling rate limits enforced by the backend 2. dealing with unexpected 4xx and 5xx errors gracefully 3. support for offline mode (i.e. when the backend is unreachable) 4. retrieving individual video details for playback 5. long polling for new/changed videos, and 6. efficient caching (e.g. using IndexedDB) to reduce the number of unnecessary requests Ping @OGTor & @ar9708 & @tryklick, FYI!
1381893
to
c51e4db
Compare
@tryklick & @OGTor Rebased this on @TOPDeveloperPB I have a hard time following your 2 comments here, it would be a lot easier if you use the GitHub review functionality + apply proper Markdown formatting to code samples. |
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.
##Your ** PeertubeVideosApi ** class looks well-structured and functional. However, here are several potential improvements and optimizations:
Request Optimization:
In your getVideos method, you're making repeated requests to the server to fetch all videos when the number of videos exceeds maxChunkSize. This can be improved by changing the logic to fetch videos incrementally until reaching the limit, reducing the number of requests.
Error Handling:
While you already handle errors, it's also worth considering error handling for connection failures or other network issues.
Type Safety:
You're already using TypeScript types for some objects and parameters, which is great. Continue to do so for all possible objects and functions.
Variable Optimization:
You're already using the class property _maxChunkSize to limit the request size, which is good. However, if you don't plan to change this value after initialization, you can make this field readonly.
Additional Checks:
If possible, add checks for input parameters to avoid invalid values.
Documentation:
Your documentation is already quite clear, but there's always room for improvement. Additional comments or explanations in the code can be helpful for future collaborators or developers.
Testing:
It's important to ensure that your code works correctly in various conditions, including error scenarios. Tests will help ensure that your code behaves as expected.
Logging:
Logging can be added to other methods besides just the request and response interceptors.
Caching:
If you frequently fetch the same data, consider caching results to avoid unnecessary requests to the server.
These improvements will help keep your code clean, efficient, and easily understandable for other developers.
Merged in #62 |
This service will be used to interact with the PeerTube backend API. It will be used to reliably retrieve videos when needed; at the moment, its functionality is limited to ...
In the future, this service will be extended to include more functionality, such as ...
Ping @OGTor & @ar9708 & @tryklick, FYI!
(Note: As an exercise, feel free to see if you can improve upon the
PeertubeVideosApi
service to make it more readable and understandable, and still passing all tests.)