search

tomoakley / freemeals.uk

MIT License
10 stars 10 forks source link

Refactor data fetch #97

Open lmjcbs opened 3 years ago

lmjcbs commented 3 years ago

Suggested refactor.

Extracted the fetch request logic to its own hook with the aim of also making testing fetching logic easier going forward.

tomoakley commented 3 years ago

there's something strange going on with the auto-geolocated results; when I load it first, it loads all the results (but the geo button is active). When I click the geo button (i.e turn off geo mode) it then shows the results closest to me

lmjcbs commented 3 years ago

@tomoakley So I wanted to address the points highlighted here first to ensure I was heading in the right direction before refactoring everything. GeoSearch functionality is intentionally disabled for now. I wanted to focus on getting all providers/location filtering functioning correctly, GeoSearch functionality should be a simple refactor going forward.

Re: filtering logic in app.js I've moved the check for null logic directly into the lambda func as suggested. One thing to note since renaming the webmanifest from site.webmanifest to manifest.webmanifest per the spec guidelines shared yesterday, is that the lambda func gives a 500 (Internal Server Error) in the console in addition to returning the data. Not sure what to make of this. Error during invocation: Error: Cannot find module '/freemeals.uk/built-lambda/site.webmanifest'

Re: needing a promise/async await I've refactored the hook to include async await and moved the data setting funcs in app.js into the hook state, ensuring the state setters will get called in order only once there is data to set.

Re: multiple fetches From some quick logging inside the fetch request within the useApi hook, I believe the fetch request is now only ever being called once and is no longer affected by rerenders as I've moved the hook call outside of useEffect inside App.js

One bug that I've found when testing this branch is that if you click provider{n} in all venues and then filter to a location that doesn't have at least n venues, the route remains and crashes the app. I've checked and this bug is also on master so I'm going to leave it out of this draft.