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

flatten out functions, async everything, send errors to template #2

Merged
merged 6 commits into from Jan 28, 2019

Conversation

stuartromanek
Copy link
Member

No description provided.

@stuartromanek
Copy link
Member Author

  • flatten out functions to do one thing
  • use await over then
  • send errors to the template
  • log all errors on server

Questions: @boutell what, if anything, should i be returning in catch blocks?

@boutell
Copy link
Member

boutell commented Jan 24, 2019

What I'm thinking is that only the route itself should take (req, res) and contain a try/catch and worry about sending a response in the successful and unsuccessful cases.

The rest of the methods can then be simple async functions that return something you want, giving you the full benefit of async/await - "return" actually means something useful. And you only need the one "catch" to handle any exception or rejected promise at any point.

Want to finish it up together?

@stuartromanek
Copy link
Member Author

@boutell take another look? If still not right, let's get together IRL

@boutell
Copy link
Member

boutell commented Jan 26, 2019

That's the stuff!

Two small things:

  • I don't think you need encodeUri, but if you are going to use it, be consistent. Right now you are using it when setting cache but not when getting cache so the cache is going to miss all the time for urls changed by it.

  • On reflection, the request calls for individual urls deserve their own try/catch so you can put in a reasonable error response for each url rather than flunking all of them. Everywhere else though the one shared try block is working great.

@stuartromanek
Copy link
Member Author

fixed up the encodeUri consistency (i ran into the need for encoding when scraping a bunch of OpenMuseum urls with special characters in the names of artwork).

To the second point, this was refactored to only make one request, even though it is formatted as an array. If that request errors it bubbles to the top catch and sends a (newly detailed) http error. Seems good?

@boutell
Copy link
Member

boutell commented Jan 28, 2019 via email

@stuartromanek
Copy link
Member Author

singular'ified

@stuartromanek stuartromanek merged commit 89149c6 into master Jan 28, 2019
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.

None yet

2 participants