348 add timeout to fetch requests #473
Conversation
src/js/components/Fetch.js
Outdated
| @@ -1,6 +1,8 @@ | |||
| import React from 'react'; | |||
| import PropTypes from 'prop-types'; | |||
|
|
|||
| const TIMEOUT = 10000; // 10 seconds | |||
There was a problem hiding this comment.
Could you refactor it so that Fetch accepts timeout as a prop and defaults to 10s?
|
So I updated the PR with timeout getting passed as a prop, but I found that timing out seems to cause #365: Will do some more digging into how we can fix this 👍 |
|
@rajkambo were you able to fix the bug? |
|
@bogas04 just mulling over what would be the best way to handle the issue. So the root cause of this error and #365 is the same -- a trying to access fields from a the null "data" object here: https://github.com/KhalisFoundation/sttm-web/blob/master/src/js/pages/Search/index.js#L44. This is caused by the error being null here: https://github.com/KhalisFoundation/sttm-web/blob/master/src/js/pages/PageLoader.js#L17. Since error the error is null here data (null) is passed on to the children and a null exception occurs leading to the generic: https://github.com/KhalisFoundation/sttm-web/blob/master/src/js/Root.js#L29. In my specific case (on timed out Fetch requests), I could modify the setTimeout call to have a third argument and make it something like "Fetch request timed out", this would then ensure that error isn't null as it is passed on here: https://github.com/KhalisFoundation/sttm-web/pull/473/files#diff-10ef1ed613a66115bbbefad359a37a6cR67. In this case we end up back at: https://github.com/KhalisFoundation/sttm-web/blob/master/src/js/pages/PageLoader.js#L17. Where an error is thrown when "error" isn't null. And since this error doesn't get caught anywhere we end up right here: https://github.com/KhalisFoundation/sttm-web/blob/master/src/js/Root.js#L29. And in since the error goes uncaught, we end up in a state where we can't click anything.. Going to do some more reading on how exactly error handling works in React, but would be great if you had any suggestions :) |
|
Update: I think I got the ideal fix! Will commit soon 👍 |
| q: PropTypes.string.isRequired, | ||
| type: PropTypes.number.isRequired, | ||
| source: PropTypes.string.isRequired, | ||
| children: PropTypes.node.isRequired, |
There was a problem hiding this comment.
Wasn't sure about the type of "children" but read this: https://stackoverflow.com/questions/42122522/reactjs-what-should-the-proptypes-be-for-this-props-children and settled on "node".
| @@ -0,0 +1,58 @@ | |||
| import React from 'react'; | |||
There was a problem hiding this comment.
Created this component for the error handling approach suggested here: https://reactjs.org/blog/2017/07/26/error-handling-in-react-16.html.
|
@bogas04 took a while to dig into this issue, and I fixed this specific error case, but it looks like we need to fix error handling as a whole in the project (as the catch all we had in Root.js isn't going to cut it). I left a detailed comment in #365 on why the links aren't working and what I think we need to do to reform our error handling👍 |
|
@bogas04 could I get the thumbs up when you get the time 👍 Would be great to wrap this one up :D |
|
@rajkambo do we need HandleSearchError or GenericError catches the fetch timeout fine? |
|
I added the logic at our single ErrorBoundary (Layout) to keep logic simple. @rajkambo thank you so much!
|
* Fix #449 (#453) * Webpack Config change + Bug Fix * Issue-449: Clean-up * Acknowledged review comments * Moved static content to 'public' directory * Undo last commit * Acknolwledged review comments * Move served files to public folder (#457) * Webpack Config change + Bug Fix * Issue-449: Clean-up * Acknowledged review comments * Moved static content to 'public' directory * Acknowledged review comments * Fix #400: Debounce font size (#470) * Debounce font size input Fixes #400 uses https://www.npmjs.com/package/redux-debounced * update package-lock * Update constants.js Update spelling of 'unusual' on error page * Fix #465 * Add location proptypes * Fix #478 change prepush module to husky (#480) * 478 change prepush to husky test1 * 478 change prepush to husky test2 * 478 change prepush to husky test3 * Preconnect banidb and polyfill.io * Fix #482 * added content type header (needed for standards and CDN) * added charset.. I am unsure if text/html is right or application/javascript or application/json * revert accidental push * moved content header to get * on advice of @bogas04 * trying application/json content type as used by api * Fix #491 #484 #489 (#498) * Fixes #365 and #496 (#499) <!-- Before submitting a PR, we would like you to confirm the following: --> * [ ] <!-- I --> Passed [sanity tests](http://bit.ly/sttm-sanity-tests). * [ ] <!-- I --> Ran `npm test` & fixed newly introduced lint errors. * [ ] <!-- I --> Checked console for errors. <!-- New to markdown? Simply put an 'x' between [ ] to check it. Like [x] this. (No spaces). * 1.1.4 * Add Remote Sync Feature (#481) * Initial sync implementation * Listen to shabadid+highlight from desktop * Cleanup * Use realy sync API * Fix API URL * Add Sync Link to Header * Add Vaars to Index * Show a toast on Sync Connection * Sundar gutka (#483) * Initial commit to fix #477 * Add SG link to header * Update infrastructure info and deploy script (#494) * Fix #348 add timeout to fetch requests (#473) * Add timeout to fetch requests * 348 add comments to fetch request with timeout * 348 add ability to pass timeout as prop into Fetch component * Fix unclickable links after fetching error * Use GenericError for timeout errors * Fix API incompatibilities with BaniDB v2 * Fix #490 * Auto close Baanies Sidebar on click, add app promo * Add link to previously read ang on Home page * Fix #460 * Fix #471 * Throttle onScroll event for ScrollToTop button * Disable smooth scrolling for long pages * Hide Meta/Footer, remove padding, for SG * Fix #476 (#505) * Fix #476 * Support unicode and other search types * Fix a special case * Analytics for SehajPaathLink * Fix #248 Add reset to font and display options (#501) * Add reset to font and display options * Add reset to font and display options --fix proptype warning * Add compound action to reset display and font store values * 248 added redux thunk dependency to handle dispatch of async events * 248 add redux thunk dependency to handle dispatch of async events * 248 add string literals to constants file * Cleanup * Fix a regression with SearchForm not reacting to URL changes * Romanized limit to 2, improve green toast * Fix UI issues with Reset button * Use route based navigation for SundarGutka * Support dark mode better * Use list view * Fix #511 * Remove autoFocus from Sundar Gutka search * Fix #506 * Use consistent highlight method between dark and light mode * Fix #448 Word break for few cases - Sihari, Bihari, etc. (#454) * #448 - Fix: Word break for few cases - Sihari, Bihari and other Right side matras along with Bindi, Mukta - Many are left * remove: css - word break - not needed any more * Fix: combination of matras, half char, bindi tippi * Fix: larivaar color highlight * #19: Fix: larivar word break issue o mobile screen, both for Punjabi font and Unicode Font * #448: Moved code to util * #448: Moved code to util - assist color * #448: Larivar moved to separate folder * Fix: sanity test - Bhai Gurdaas Ji Check: * #448: remove unwanted code * #448: Update snapshot tests * Rename componentWillMount - deprecated/renamed in React v 16.4.0 * removed unsafe method * fix: variable name * #19 - Fix: missing style for wordBreakException case, add style instead of class * Fix: merge conflict * Use React Component for word, const, cleanup * Hide Sync from menu * Fix #515
* Fix #449 (#453) * Webpack Config change + Bug Fix * Issue-449: Clean-up * Acknowledged review comments * Moved static content to 'public' directory * Undo last commit * Acknolwledged review comments * Move served files to public folder (#457) * Webpack Config change + Bug Fix * Issue-449: Clean-up * Acknowledged review comments * Moved static content to 'public' directory * Acknowledged review comments * Fix #400: Debounce font size (#470) * Debounce font size input Fixes #400 uses https://www.npmjs.com/package/redux-debounced * update package-lock * Update constants.js Update spelling of 'unusual' on error page * Fix #465 * Add location proptypes * Fix #478 change prepush module to husky (#480) * 478 change prepush to husky test1 * 478 change prepush to husky test2 * 478 change prepush to husky test3 * Preconnect banidb and polyfill.io * Fix #482 * added content type header (needed for standards and CDN) * added charset.. I am unsure if text/html is right or application/javascript or application/json * revert accidental push * moved content header to get * on advice of @bogas04 * trying application/json content type as used by api * Fix #491 #484 #489 (#498) * Fixes #365 and #496 (#499) <!-- Before submitting a PR, we would like you to confirm the following: --> * [ ] <!-- I --> Passed [sanity tests](http://bit.ly/sttm-sanity-tests). * [ ] <!-- I --> Ran `npm test` & fixed newly introduced lint errors. * [ ] <!-- I --> Checked console for errors. <!-- New to markdown? Simply put an 'x' between [ ] to check it. Like [x] this. (No spaces). * 1.1.4 * Add Remote Sync Feature (#481) * Initial sync implementation * Listen to shabadid+highlight from desktop * Cleanup * Use realy sync API * Fix API URL * Add Sync Link to Header * Add Vaars to Index * Show a toast on Sync Connection * Sundar gutka (#483) * Initial commit to fix #477 * Add SG link to header * Update infrastructure info and deploy script (#494) * Fix #348 add timeout to fetch requests (#473) * Add timeout to fetch requests * 348 add comments to fetch request with timeout * 348 add ability to pass timeout as prop into Fetch component * Fix unclickable links after fetching error * Use GenericError for timeout errors * Fix API incompatibilities with BaniDB v2 * Fix #490 * Auto close Baanies Sidebar on click, add app promo * Add link to previously read ang on Home page * Fix #460 * Fix #471 * Throttle onScroll event for ScrollToTop button * Disable smooth scrolling for long pages * Hide Meta/Footer, remove padding, for SG * Fix #476 (#505) * Fix #476 * Support unicode and other search types * Fix a special case * Analytics for SehajPaathLink * Fix #248 Add reset to font and display options (#501) * Add reset to font and display options * Add reset to font and display options --fix proptype warning * Add compound action to reset display and font store values * 248 added redux thunk dependency to handle dispatch of async events * 248 add redux thunk dependency to handle dispatch of async events * 248 add string literals to constants file * Cleanup * Fix a regression with SearchForm not reacting to URL changes * Romanized limit to 2, improve green toast * Fix UI issues with Reset button * Use route based navigation for SundarGutka * Support dark mode better * Use list view * Fix #511 * Remove autoFocus from Sundar Gutka search * Fix #506 * Use consistent highlight method between dark and light mode * Fix #448 Word break for few cases - Sihari, Bihari, etc. (#454) * #448 - Fix: Word break for few cases - Sihari, Bihari and other Right side matras along with Bindi, Mukta - Many are left * remove: css - word break - not needed any more * Fix: combination of matras, half char, bindi tippi * Fix: larivaar color highlight * #19: Fix: larivar word break issue o mobile screen, both for Punjabi font and Unicode Font * #448: Moved code to util * #448: Moved code to util - assist color * #448: Larivar moved to separate folder * Fix: sanity test - Bhai Gurdaas Ji Check: * #448: remove unwanted code * #448: Update snapshot tests * Rename componentWillMount - deprecated/renamed in React v 16.4.0 * removed unsafe method * fix: variable name * #19 - Fix: missing style for wordBreakException case, add style instead of class * Fix: merge conflict * Use React Component for word, const, cleanup * Hide Sync from menu * Fix #515 * Fix webpack typo * 1.1.5
npm test& fixed newly introduced lint errors.