-
Notifications
You must be signed in to change notification settings - Fork 94
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
Add infinite loop prevention #165
base: next
Are you sure you want to change the base?
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 7592e1e:
|
Codecov Report
@@ Coverage Diff @@
## next #165 +/- ##
=======================================
Coverage 98.62% 98.62%
=======================================
Files 9 9
Lines 436 436
Branches 150 150
=======================================
Hits 430 430
Misses 6 6 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides that, it looks good to me 👍
packages/react-async/src/useAsync.js
Outdated
@@ -20,6 +21,9 @@ const useAsync = (arg1, arg2) => { | |||
const lastOptions = useRef(undefined) | |||
const lastPromise = useRef(neverSettle) | |||
const abortController = useRef({ abort: noop }) | |||
const [preventLoop] = useState(loopPreventer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd use useRef
instead of useState
here and keep preventLoop in a ref, as it is not really application state. But that's more a thing of semantics and will not affect anything.
Maybe move it out in a custom hook like this:
function useLoopPreventer() {
const preventLoop = useRef();
if (!preventLoop.current) {
preventLoop.current = loopPreventer();
}
return preventLoop.current;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can't be a hook, because <Async>
is backwards compatible to React 16.3 which doesn't support hooks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I used useState
is for its lazy initial value. I give it a function that will only be invoked once, which is exactly what I want. It can be done with useRef but requires more code and doesn't take a resolver function for the initial value, so we'd be creating a new function each render, which we don't need.
Description
This adds a mechanism that detects 'infinite' loops, and throws an error if that happens. This makes sure that if a user were to inadvertently create a loop, for example by defining
promiseFn
inline, the browser won't hang but the app will crash instead.This fixes #34
Checklist
Make sure you check all the boxes. You can omit items that are not applicable.
<Async>
anduseAsync()