refactor: Update all API fetches to use onebusaway-sdk #35
Conversation
There was a problem hiding this comment.
I love where you're going with this! I think you can simplify it further as I laid out in the review comments. One of the hardest things to do effectively in software engineering is to figure out the right level of abstraction, especially when you need to unwind a level of abstraction that was put in place before.
I had introduced a layer of abstraction only because I didn't have the well-developed SDK that you're now introducing. With that integrated into the project, I think we're a lot better off talking directly to that instead of the intermediate layer.
| import { error, json } from '@sveltejs/kit'; | ||
| import { PUBLIC_OBA_SERVER_URL as baseURL } from '$env/static/public'; | ||
| import { PRIVATE_OBA_API_KEY as apiKey } from '$env/static/private'; | ||
| import shape from '../../../../../lib/RestAPI/shape.js'; |
There was a problem hiding this comment.
Check out the $lib alias on https://kit.svelte.dev/docs/configuration and use it here to avoid this sort of deep nesting.
There was a problem hiding this comment.
Actually, I was using the $lib alias, but I noticed that when I used it, the autocomplete and types for oba disappeared. So, I switched to a relative path as a temporary solution.
However, after reading the docs, I think we can add a jsconfig.json file to define $lib, so the compiler will recognize it properly.
| console.error('Error fetching shape data:', err); | ||
| throw error(500, 'Unable to fetch shape data.'); | ||
| } | ||
| const shapeId = params.shapeId; |
There was a problem hiding this comment.
I'm not certain that the abstraction of having $lib/RestAPI/shape.js buys us anything here in terms of decreased complexity. I think I'd rather see that logic expressed here without the intermediary step. And I think the same should probably go for the other files under RestAPI unless they are DRYing up a lot of extra logic.
| /** @type {import('./$types').RequestHandler} */ | ||
|
|
||
| import stop from '../../../../../lib/RestAPI/stop'; |
There was a problem hiding this comment.
Use the $lib alias again, but import obaSdk and use that directly.
| export async function GET({ params }) { | ||
| return arrivalDepartureAPI(params.id); | ||
| const stopID = params.id; | ||
| return arrivalsAndDeparturesForStop(stopID); |
There was a problem hiding this comment.
import obaSdk and use that directly.
There was a problem hiding this comment.
I replied in the last review about the options we have.
|
|
||
| const data = await response.json(); | ||
| return json(data); | ||
| return stopsForLocation(queryParams); |
There was a problem hiding this comment.
import obaSdk and use that directly.
There was a problem hiding this comment.
I replied in the last review about the options we have.
| import stopAPI from '$lib/RestAPI/stop.js'; | ||
| import arrivalDepartureAPI from '$lib/RestAPI/arrivalsAndDeparturesForStop.js'; | ||
| import arrivalsAndDeparturesForStop from '../../../lib/RestAPI/arrivalsAndDeparturesForStop.js'; | ||
| import stop from '../../../lib/RestAPI/stop.js'; | ||
|
|
||
| export async function load({ params }) { |
There was a problem hiding this comment.
I think this function will be a lot clearer if we use your obaSdk abstraction directly, instead of the layer on top of it.
There was a problem hiding this comment.
I was considering deleting the RestApi folder and using the SDK directly in the api/routes folder.
Actually, I initially implemented it this way but reverted back because I noticed that src/routes/stops uses some functions from RestApi. If we want to use the SDK directly without any level of abstraction, we will need to check response.code !== 200 and handle errors in every file. So, I think the abstraction is beneficial for handling errors and other repetitive tasks. What do you think?
To sum up the current options:
1 - Delete the RestApi folder and use the SDK directly in src/routes, adding error handling in every fetch throughout the app.
2 - Keep the RestApi and continue with this pattern of abstraction.
There was a problem hiding this comment.
So, we can use abstractions as level of error handling and make it reusable
There was a problem hiding this comment.
That's a fair point. I spent some time thinking about this, and I think that a good middle ground would be to add a new function to obaSdk.js called handleOBAResponse() (or something similar—I'm open to suggestions on naming 😄) that takes care of the repeated handling of 200 and non-200 status codes that we see throughout the API layer:
return handleOBAResponse(response, 'Unable to fetch shape.');
function handleOBAResponse(response, errorMessage) {
if (response.code !== 200) {
return error(500, errorMessage);
}
return json(response);
}
There was a problem hiding this comment.
I think this is a great solution. I was thinking that we could add a pattern and use it across all API fetching.
Here's what I will do:
1- Complex APIs will have their logic separated into the RestApi folder.
2- For simple API fetching, like shape, I will use ObaSdk directly in the routes/api folder.
3- I will add an obaHandleApi function and use it for any API calls in the app
There was a problem hiding this comment.
I have added the new modifications and removed the RestApi folder.
|
@Ahmedhossamdev is this ready for review? |
Yes |
This PR updates all REST API methods to use onebusaway-sdk, making the code cleaner and easier to maintain. By using the SDK, we improve consistency and reliability in API interactions.
This PR also changes the way APIs are fetched to enforce a pattern that ensures consistency across all routes.
Additionally, this PR introduces a singleton obaSdk to avoid multiples initializations and improve performance.
Please do
npm i [email protected].
This is the latest version of the SDK.