Adding the list of exchanges and services to the get ecash page. The data is being pulled from the scorecard.cash api using the recently added methods, and sorted on the same scoring criteria that scorecard.cash uses so better services are listed first
Details
- Reviewers
bytesofman Fabien - Group Reviewers
Restricted Project - Commits
- rABC78853044d9dd: [e.cash] Fetch exchanges and services and render on get ecash page
preview the site and go to the get ecash page
check everything looks good and all the links work
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- get-ecash-content
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 24031 Build 47672: Build Diff Build 47671: arc lint + arc unit
Event Timeline
web/e.cash/utils/pageUtils.js | ||
---|---|---|
2 ↗ | (On Diff #40716) | This is not related to pages. In any case you should really think twice before creating a util/ directory. It's a smell of a bad design, 99% of the time you can find a better place for the code. Like, score.js ? Next to scoringCriteria.js ? And why not making it a single file so it's self contained and can be tested easily ? |
Good suggestions in general, added.
As for the point about the api failing, getStaticProps only runs at build time, and if the call fails it will abort the build and throw an error, so the getScores function wont be called
web/e.cash/data/__tests__/scores.test.js | ||
---|---|---|
39 ↗ | (On Diff #40743) | kinda confusing description 'should leave input array unchanged if length is divisible by 3' |
web/e.cash/data/scores.js | ||
106 ↗ | (On Diff #40743) | |
107 ↗ | (On Diff #40743) | would be nice to know this only runs on app build and not on page load |
web/e.cash/pages/get-ecash.js | ||
112 ↗ | (On Diff #40743) |
For this unit test, you will want to mock fetch , i.e. one test case where you mock expected responses, another where you mock error responses and show that the error is thrown |
web/e.cash/pages/get-ecash.js | ||
---|---|---|
112 ↗ | (On Diff #40743) | any other guidance/ideas for testing getstaticprops? Having trouble finding good docs on mocking fetch, and Im not sure if testing getstatic props will behave the same as a regular function since its part of the next.js library... |
web/e.cash/pages/get-ecash.js | ||
---|---|---|
112 ↗ | (On Diff #40743) | options Cashtab uses (2) |
web/e.cash/data/scores.js | ||
---|---|---|
176 ↗ | (On Diff #40767) | if this is only used in this function, just hardcode it into the function or define it at the top of the function defining it outside here creates an unecessary global variable |
184 ↗ | (On Diff #40767) | what does responses look like here in the event of an error? We have a unit test which is passing, but this is still a weird way to handle it. more robust would be let responses; try { responses = await Promise.all([..]) } catch (err) { throw new Error(`Failed to fetch scorecard API response`); } |
190 ↗ | (On Diff #40767) | getScores requires the data to be a very specific shape, e.g. have a .criteria key etc. No error handling for this Possible for the API server DB to fail and return an empty array or object Should have some kind of validation and throw an error if the data is not the shape you need |
web/e.cash/data/scores.js | ||
---|---|---|
190 ↗ | (On Diff #40772) | this isn't the approach we want to take here. While this would work on the very specific possible responses of an empty array or empty object, what we want to show is that an error is thrown on any response that is not of the required shape. It doesn't need to be too complicated to get through this, though. instead of return props: {exchanges: makeDivisibleByThree(getScores..} just use the same try...catch block you already have to wrap the API call try { // Make API calls const responses = apiCallsMadeSuccessfully // Try to determine props from the api result const props = { exchanges: makeDivisibleByThree( getScores(responses[0], exchangeScoringCriteria), ), instantExchanges: makeDivisibleByThree( getScores(responses[1], instantExchangeScoringCriteria), ), services: makeDivisibleByThree( getScores(responses[2], servicesScoringCriteria), ), } } catch (err) { throw err; } |
Build Bitcoin ABC Diffs / Diff Testing (preview-e.cash) passed.
Preview is available at http://54.39.19.73:41547 for the next 60 minutes.
This diff is doing too much things at the same time. You should first add the score.js file and functions with its tests, then add the exchanges to the website.
web/e.cash/data/__mocks__/scoresMock.js | ||
---|---|---|
1 | I suppose it's generated ? If so please add a comment with instructions to regenerate and add a @generated mark | |
web/e.cash/data/__tests__/scores.test.js | ||
1 | The header is missing, but I think this is the case for most if not all the files in the repo. | |
web/e.cash/data/scores.js | ||
13 | Use proper doxygen layout: /** * My wonderful comment. * Could very well span on multiple lines. */ | |
111 | Remove or the doc won't link to the function | |
158 | dito, please go over the whole file |
web/e.cash/next.config.js | ||
---|---|---|
6 ↗ | (On Diff #40822) | its a next js thing for images that have an outside url. have to specify it here otherwise get this error https://nextjs.org/docs/messages/next-image-unconfigured-host |
Build Bitcoin ABC Diffs / Diff Testing (preview-e.cash) passed.
Preview is available at http://51.68.37.192:41178 for the next 60 minutes.
web/e.cash/pages/get-ecash.js | ||
---|---|---|
111 ↗ | (On Diff #40822) | remove line break in between function comments and function |
web/e.cash/next.config.js | ||
---|---|---|
12 ↗ | (On Diff #40827) | Is that the whole CORS config or does the server need to handle this as well ? If so you want to add it to the nginx.conf |
web/e.cash/next.config.js | ||
---|---|---|
12 ↗ | (On Diff #40827) | afaik thats all we need here. It ensures the next.js image optimization API isn't exploited |
web/e.cash/next.config.js | ||
---|---|---|
12 ↗ | (On Diff #40827) | OK let's move on for now and we'll figure that out as needed |