Skip to content
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

refactor: Update all API fetches to use onebusaway-sdk #35

Merged
merged 34 commits into from
Aug 30, 2024
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
536c4a4
chore: Update onebusaway-sdk dependency to version 0.1.0-alpha.40
Ahmedhossamdev Aug 22, 2024
124499f
Refactor: Use a singleton instance for OBA SDK in obaSdk.js
Ahmedhossamdev Aug 22, 2024
dc65dad
refactor: Use onebusaway sdk for API calls in arrivalsAndDeparturesFo…
Ahmedhossamdev Aug 22, 2024
e263243
feat: Add shape.js for retrieving shape data from the sdk
Ahmedhossamdev Aug 22, 2024
f2d119e
refactor: Update stop.js to use obaSdk for retrieving stop data
Ahmedhossamdev Aug 22, 2024
47ecf1d
feat: Add stopsForLocation to use obaSD to fetch data
Ahmedhossamdev Aug 22, 2024
3a43aac
refactor: Update tripDetails.js to use obaSdk for retrieving trip det…
Ahmedhossamdev Aug 22, 2024
1ff8e57
refactor: Use `arrivalsAndDeparturesForStop API helper in GET handler
Ahmedhossamdev Aug 22, 2024
626330c
refactor: Update GET handler to use shape function for retrieving sha…
Ahmedhossamdev Aug 22, 2024
96d7e32
feat: Update GET handler to use stop function for retrieving stop data
Ahmedhossamdev Aug 22, 2024
091675d
feat: Update stops-for-location to use stopsForLocation function
Ahmedhossamdev Aug 22, 2024
628ebff
refactor: Update trip-details GET handler to use obaSdk and tripDetai…
Ahmedhossamdev Aug 22, 2024
9c16162
refactor: Update imports paths
Ahmedhossamdev Aug 22, 2024
e6218eb
refactor: import paths and remove unnecessary imports
Ahmedhossamdev Aug 22, 2024
5a5d525
refactor: Update import paths
Ahmedhossamdev Aug 22, 2024
8c5d4f5
feat: add jsconfig.json file so the compiler know about lib files
Ahmedhossamdev Aug 23, 2024
df6329b
refactor: Update import paths
Ahmedhossamdev Aug 23, 2024
aaf57a6
refactor: Update import paths
Ahmedhossamdev Aug 23, 2024
f4e4161
refactor: Update import paths
Ahmedhossamdev Aug 23, 2024
176224d
refactor: Update import paths
Ahmedhossamdev Aug 23, 2024
966099c
refactor: Update import paths
Ahmedhossamdev Aug 23, 2024
d8ee6d4
refactor: extend tsconfig.json
Ahmedhossamdev Aug 24, 2024
cd7bacc
refactor: add alias for lib folder
Ahmedhossamdev Aug 24, 2024
46a33b1
refactor: add handleOBAResponse function to handle response errors
Ahmedhossamdev Aug 24, 2024
366fe9e
refactor: Remove unused files
Ahmedhossamdev Aug 24, 2024
883bf46
refactor: use sdk directly for api interactions
Ahmedhossamdev Aug 24, 2024
ead98f8
refactor: use sdk directly for api interactions
Ahmedhossamdev Aug 24, 2024
5422d78
refactor: use sdk directly for api interactions
Ahmedhossamdev Aug 24, 2024
d9fb334
refactor: use sdk directly for api interactions
Ahmedhossamdev Aug 24, 2024
2b4e53d
refactor: use sdk directly for api interactions
Ahmedhossamdev Aug 24, 2024
17b0671
refactor: use sdk directly for api interactions
Ahmedhossamdev Aug 24, 2024
a82174e
refactor: update to import error from '@sveltejs/kit'
Ahmedhossamdev Aug 24, 2024
8a294bf
Merge branch 'main' into refactor/sdk-intergration
Ahmedhossamdev Aug 24, 2024
c973c9e
Updates OBA SDK to latest version
aaronbrethorst Aug 30, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,6 @@
"type": "module",
"dependencies": {
"@fortawesome/free-regular-svg-icons": "^6.6.0",
"onebusaway-sdk": "^0.1.0-alpha.31"
"onebusaway-sdk": "^0.1.0-alpha.40"
}
}
13 changes: 2 additions & 11 deletions src/lib/RestAPI/arrivalsAndDeparturesForStop.js
Original file line number Diff line number Diff line change
@@ -1,17 +1,8 @@
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 oba from '../obaSdk';

import onebusaway from 'onebusaway-sdk';

const oba = new onebusaway({
baseURL,
apiKey
});

export default async function (stopID) {
export default async function arrivalsAndDeparturesForStop(stopID) {
const response = await oba.arrivalAndDeparture.list(stopID);

if (response.code !== 200) {
error(500, 'Unable to fetch arrivals-and-departures-for-stop.');
}
Expand Down
11 changes: 11 additions & 0 deletions src/lib/RestAPI/shape.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import oba from '../obaSdk';
import { error, json } from '@sveltejs/kit';
export default async function shape(shapeId) {
const response = await oba.shape.retrieve(shapeId);

if (response.code !== 200) {
return error(500, 'Unable to fetch shape.');
}

return json(response);
}
16 changes: 3 additions & 13 deletions src/lib/RestAPI/stop.js
Original file line number Diff line number Diff line change
@@ -1,19 +1,9 @@
import oba from '../obaSdk';
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 onebusaway from 'onebusaway-sdk';

const oba = new onebusaway({
baseURL,
apiKey
});

export default async function (stopID) {
export default async function stop(stopID) {
const response = await oba.stop.retrieve(stopID);

if (response.code !== 200) {
error(500, 'Unable to fetch arrivals-and-departures-for-stop.');
error(500, 'Unable to fetch stop.');
}

return json(response);
Expand Down
11 changes: 11 additions & 0 deletions src/lib/RestAPI/stops-for-location.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import oba from '../obaSdk';
import { error, json } from '@sveltejs/kit';
export default async function stopsForLocation(queryParams) {
const response = await oba.stopsForLocation.list(queryParams);

if (response.code !== 200) {
error(500, 'Unable to fetch stops-for-location.');
}

return json(response);
}
24 changes: 5 additions & 19 deletions src/lib/RestAPI/tripDetails.js
Original file line number Diff line number Diff line change
@@ -1,24 +1,10 @@
import oba from '../obaSdk';
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';

export async function GET({ params, url }) {
const { tripId } = params;
const vehicleId = url.searchParams.get('vehicleId');
const serviceDate = url.searchParams.get('serviceDate');

let apiURL = `${baseURL}/api/where/trip-details/${tripId}.json?key=${apiKey}`;

if (vehicleId) apiURL += `&vehicleId=${vehicleId}`;
if (serviceDate) apiURL += `&serviceDate=${serviceDate}`;

const response = await fetch(apiURL);

if (!response.ok) {
export default async function tripDetails(tripID, queryParams) {
const response = await oba.tripDetails.retrieve(tripID, queryParams);
if (response.code !== 200) {
error(500, 'Unable to fetch trip-details.');
return;
}

const data = await response.json();
return json(data);
return json(response);
}
10 changes: 10 additions & 0 deletions src/lib/obaSdk.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import onebusaway from 'onebusaway-sdk';
import { PUBLIC_OBA_SERVER_URL as baseURL } from '$env/static/public';
import { PRIVATE_OBA_API_KEY as apiKey } from '$env/static/private';

const oba = new onebusaway({
baseURL,
apiKey
});

export default oba;
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import arrivalDepartureAPI from '$lib/RestAPI/arrivalsAndDeparturesForStop';
import arrivalsAndDeparturesForStop from '$lib/RestAPI/arrivalsAndDeparturesForStop';

/** @type {import('./$types').RequestHandler} */

export async function GET({ params }) {
return arrivalDepartureAPI(params.id);
const stopID = params.id;
return arrivalsAndDeparturesForStop(stopID);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import obaSdk and use that directly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I replied in the last review about the options we have.

24 changes: 3 additions & 21 deletions src/routes/api/oba/shape/[shapeId]/+server.js
Original file line number Diff line number Diff line change
@@ -1,24 +1,6 @@
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';

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check out the $lib alias on https://kit.svelte.dev/docs/configuration and use it here to avoid this sort of deep nesting.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

oba-type

However, after reading the docs, I think we can add a jsconfig.json file to define $lib, so the compiler will recognize it properly.

export async function GET({ params }) {
const { shapeId } = params;

let apiURL = `${baseURL}/api/where/shape/${shapeId}.json?key=${apiKey}`;

try {
const response = await fetch(apiURL);
console.log('response:', response);

if (!response.ok) {
throw new Error(`HTTP error! status: ${response.status}`);
}

const data = await response.json();
return json(data);
} catch (err) {
console.error('Error fetching shape data:', err);
throw error(500, 'Unable to fetch shape data.');
}
const shapeId = params.shapeId;
return shape(shapeId);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
8 changes: 5 additions & 3 deletions src/routes/api/oba/stop/[stopID]/+server.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import stopAPI from '$lib/RestAPI/stop';

/** @type {import('./$types').RequestHandler} */

import stop from '../../../../../lib/RestAPI/stop';

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the $lib alias again, but import obaSdk and use that directly.

export async function GET({ params }) {
return stopAPI(params.stopID);
const stopID = params.stopID;
return stop(stopID);
}
23 changes: 8 additions & 15 deletions src/routes/api/oba/stops-for-location/+server.js
Original file line number Diff line number Diff line change
@@ -1,21 +1,14 @@
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 stopsForLocation from '../../../../lib/RestAPI/stops-for-location.js';

/** @type {import('./$types').RequestHandler} */
export async function GET({ url }) {
const lat = url.searchParams.get('lat');
const lng = url.searchParams.get('lng');
const apiURL = `${baseURL}/api/where/stops-for-location.json?key=${apiKey}&lat=${lat}&lon=${lng}`;
const response = await fetch(apiURL);
const lat = +url.searchParams.get('lat');
const lng = +url.searchParams.get('lng');

if (!response.ok) {
error(500, 'Unable to fetch stops-for-location.');
return;
}
const queryParams = {
lat: lat,
lon: lng
};

const data = await response.json();
return json(data);
return stopsForLocation(queryParams);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import obaSdk and use that directly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I replied in the last review about the options we have.

43 changes: 13 additions & 30 deletions src/routes/api/oba/trip-details/[tripId]/+server.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
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 tripDetails from '../../../../../lib/RestAPI/tripDetails.js';

export async function GET({ params, url }) {
const { tripId } = params;
Expand All @@ -11,34 +9,19 @@ export async function GET({ params, url }) {
const includeStatus = url.searchParams.get('includeStatus') || 'true';
const time = url.searchParams.get('time');

let apiURL = `${baseURL}/api/where/trip-details/${tripId}.json?key=${apiKey}`;
const queryParams = {
includeTrip,
includeSchedule,
includeStatus
};

if (serviceDate) apiURL += `&serviceDate=${serviceDate}`;
apiURL += `&includeTrip=${includeTrip}`;
apiURL += `&includeSchedule=${includeSchedule}`;
apiURL += `&includeStatus=${includeStatus}`;
if (time) apiURL += `&time=${time}`;

try {
const response = await fetch(apiURL);

if (!response.ok) {
throw new Error(`HTTP error! status: ${response.status}`);
}

const data = await response.json();

// Ensure stops are included in the references
if (!data.data.references || !data.data.references.stops) {
// If stops are not included, we might need to fetch them separately
// This is a placeholder for that logic
data.data.references = data.data.references || {};
data.data.references.stops = []; // Fetch stops here if needed
}
if (time) {
queryParams.time = time;
}

return json(data);
} catch (err) {
console.error('Error fetching trip details:', err);
throw error(500, 'Unable to fetch trip details.');
if (serviceDate) {
queryParams.serviceDate = serviceDate;
}

return tripDetails(tripId, queryParams);
}
8 changes: 4 additions & 4 deletions src/routes/stops/[stopID]/+page.server.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
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 }) {
const stopID = params.stopID;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this function will be a lot clearer if we use your obaSdk abstraction directly, instead of the layer on top of it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, we can use abstractions as level of error handling and make it reusable

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
}

Copy link
Member Author

@Ahmedhossamdev Ahmedhossamdev Aug 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added the new modifications and removed the RestApi folder.

@aaronbrethorst

const stopResponse = await stopAPI(stopID);
const stopResponse = await stop(stopID);
const stopBody = await stopResponse.json();
const arrivalsAndDeparturesResponse = await arrivalDepartureAPI(stopID);
const arrivalsAndDeparturesResponse = await arrivalsAndDeparturesForStop(stopID);
const arrivalsAndDeparturesResponseJSON = await arrivalsAndDeparturesResponse.json();

return {
Expand Down
Loading