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

Improve caching and error handling in hooks.server.js #172

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

SophiaLi20
Copy link

This PR makes some improvements to hooks.server.js to optimize caching, improve error handling, and clean up the code for better readability.

Key Changes:

  • Added cache expiration (5 minutes) to ensure we're not serving outdated route data.
  • Improved error handling in fetchRoutesData() to log issues properly without breaking execution.
  • Optimized performance by ensuring we only fetch new data when needed instead of on every request.
  • Cleaned up the code with better comments, clearer variable names, and a more structured approach to handling agency data.

Why?

  • Previously, routesCache was never refreshed, meaning the app could serve stale data indefinitely.
  • handle() was calling preloadRoutesData() on every request, which wasn’t necessary.
  • The code lacked comments and could be a bit tricky to follow at first glance.

Testing:

  • Verified that routes are correctly cached and refreshed when needed.
  • Checked that errors are logged properly without stopping execution.
  • Ensured API calls are only made when required.

@CLAassistant
Copy link

CLAassistant commented Feb 8, 2025

CLA assistant check
All committers have signed the CLA.

@aaronbrethorst
Copy link
Member

@SophiaLi20 please sign the CLA

@SophiaLi20
Copy link
Author

Hey @aaronbrethorst , I have signed the CLA. Please let me know if any further action is required. Thanks!

Copy link
Member

@aaronbrethorst aaronbrethorst left a comment

Choose a reason for hiding this comment

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

This looks great! Would you add a couple basic tests to the project to ensure that fetchRoutesData() works as expected?

@SophiaLi20
Copy link
Author

Thanks, @aaronbrethorst ! I appreciate your feedback. I'll review your suggestions and make the necessary changes. Let me know if you have any specific concerns or if there's anything else you'd like me to address.

@SophiaLi20
Copy link
Author

Hey @aaronbrethorst ,I sincerely apologize for the delay. I was occupied with exams and could not get to this sooner. I’ve added the requested tests for fetchRoutesData() and committed the changes. When you get a chance, could you take a look and let me know if any further modifications are needed? Happy to make any updates if needed. Thanks for your patience!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants