Improved onError handling #7
Conversation
| .catch((err) => { | ||
| this.props.onError(err, bail() || 'other'); | ||
| this.abort(true); | ||
| }); |
There was a problem hiding this comment.
While I agree that we should abort (since we're no longer loading), I'm wondering whether we should also transition back to the previous location. If we abort, then we'll setState, which will cause a render(), which will render the newly matched location, but with data that failed to fetch. Perhaps that is not our concern, or maybe it is? Either way, I'd say it warrants further discussion
There was a problem hiding this comment.
Yes, that would probably be good! I'm all for this change!
Maybe it should be an option, since a scenario might be that the user reloads the page and would then get the correct resource?
There was a problem hiding this comment.
Can you make an example where for one route, the blocking fetch fails, and for another route the deferred fetch fails? I think we might want to handle these differently...
- Runs defer and blocking separately in parallel, before they where part of the same promise chain meaning if one failed the other would be discarded - Better onError the power to reload the page for example on error or go back - onCompleted now gets a arguments that will be either blocking or deferred depending on what completed
This to test the new onError system mainly.
dlmr
changed the title
|
Made the error handling more generic where users can implement it as they see fit for their use case. Example const forcePageReloadOnError = true;
const goBackOnError = false;
// Function that can be used as a setting for useRedial
function onError(err, { abort, blocking, reason, router }) {
if (process.env.NODE_ENV !== 'production') {
console.error(reason, err);
}
// We only what to do this if it was a blocking hook that failed
if (blocking) {
if (forcePageReloadOnError) {
window.location.reload();
} else if (goBackOnError) {
router.goBack();
}
// Abort current loading automatically
abort();
}
} |
By using displayName the component will keep its name even after minification
This makes parallel and non parallel behaviour consistent
This solves an issue where aborting an then quickly making a new request, resulting in that both the old and the new request was aborted
dlmr
force-pushed
the
feature/abort-on-failure
branch
from
cdf3ee0
to
cb5335d
Compare
dlmr
force-pushed
the
feature/abort-on-failure
branch
from
cb5335d
to
5c6f4a4
Compare
|
The question is if we should change |
| @@ -17,6 +17,8 @@ function hydrate(renderProps) { | |||
| } | |||
|
|
|||
| export default class RedialContext extends Component { | |||
| static displayName = 'RedialContext'; | |||
There was a problem hiding this comment.
Why do we need this? In __DEV__ it'll be inferred automatically, and in production we don't want/need the name to not be minimized?
There was a problem hiding this comment.
Correct, we would like to keep the name even in a minified build.
| .then(() => { | ||
| if (this.state.beforeTransitionCompleted) { | ||
| this.props.onCompleted('afterTransition'); | ||
| } |
There was a problem hiding this comment.
If we are in parallel-mode, won't we have a risk of calling onCompleted twice with 'afterTransition'? Here, and line L280
There was a problem hiding this comment.
True, this could probably happen currently depending how how the promises are invoked. Need to look closer at this.
| import getRoutePath from './util/getRoutePath'; | ||
|
|
||
| export default class RedialContextContainer extends React.Component { | ||
| export default class RedialContainer extends Component { | ||
| static displayName = 'RedialContainer'; |
There was a problem hiding this comment.
If we want the component to not be called something like t in React devtools.
Made sure that we only run onCompleted for afterTransition once Made sure we wait with showing error for afterTransition until beforeTransition have succeded
This will prevent the application to get stuck in a loading state and requiring manual interaction.
@PAkerstrand Does this seem like a good default for you?