-
Notifications
You must be signed in to change notification settings - Fork 11
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
Fix race conditions in UseIModelData #136
Conversation
packages/modules/imodel-browser/src/containers/iModelGrid/useIModelData.test.ts
Dismissed
Show dismissed
Hide dismissed
packages/modules/imodel-browser/src/containers/iModelGrid/useIModelData.test.ts
Dismissed
Show dismissed
Hide dismissed
packages/modules/imodel-browser/src/containers/iModelGrid/useIModelData.test.ts
Dismissed
Show dismissed
Hide dismissed
I have not tested in React 18 strict mode, but there is a chance this PR could solve #128, as that looks like it could be a race condition. The abort controller was prematurely terminated when the dependencies of the effect were changed before the request finished. |
packages/modules/imodel-browser/src/containers/iModelGrid/useIModelData.ts
Outdated
Show resolved
Hide resolved
packages/modules/imodel-browser/src/containers/iModelGrid/useIModelData.ts
Outdated
Show resolved
Hide resolved
I found some bugs when the limit is set low and the imodels don't fill the page. I presume it's related to my changes even though my changes were in the data layer. Looking into it... |
Fixed a couple more bugs.
|
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.
didnt test locally but seems ok
packages/modules/imodel-browser/src/containers/iModelGrid/useIModelData.ts
Outdated
Show resolved
Hide resolved
packages/modules/imodel-browser/src/containers/iModelGrid/useIModelData.ts
Show resolved
Hide resolved
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.
didnt test locally but seems ok
The iModelGrid component was erroneously saying that I had no iModels in my iTwin. This was happening roughly 90% of the time with my initial code, but an unrelated code change could cause it to happen <5% of the time.
The race condition came in due to the useIModelData hook having multiple useEffects that relied on the same dependencies, changed the same state, where the order mattered. React does not guarantee an order in which these effects are run.
The component was refactored and simplified to have more linear logic without race conditions.
Other bugs fixed in the process of testing my changes: