-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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 undefined controller error in tooltip plugin #11651
base: master
Are you sure you want to change the base?
Fix undefined controller error in tooltip plugin #11651
Conversation
Fix controller being undefined in tooltip plugin
@etimberg we dissalowed optional chaining because it was not stable yet and not all platforms supported it. It seems it is stable now and every major browser supports it, except for IE 11 but we do not support that ourselfs. I think we can use it now, what is your oppnion on it? |
@LeeLenaleee I'm ok to use it now that it's stable |
I have this exact issue currently and will wait for this fix to be merged. Our scenario happens when swopping between the graph on the dashboard and the page load from the graph click event multiple times, and only when this is done quickly. |
Just confirming that I also have this issue in my current codebase - in my case, it seems to be related to multiple graphs being loaded at the same time under a stress test. It's four graphs in total with four sets of data (~600 points) being drawn each. I've already staggered the loading because it wouldn't go smoothly otherwise (I first init the graphjs instance and then load in the data a short timeout later) and the missing controller happens seemingly at random - skipping over the error, all tooltips work just fine a moment later, navigating away and back to the page, I cannot reproduce the error consistently. Another weird thing is that when I add a breakpoint to where the error occurs, I can print out the content of controller just fine (it's not null), but then the error happens anyhow (it reports that controller IS null). I first thought that the fix in this PR resolved the issue, but the error ended up cropping up again, so now I'm completely unsure what causes it. |
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.
Changes Made:
- Modified the code to use optional chaining (
?.
) in place of direct property access (.
) forgetParsed
method ofcontroller
.
Review:
-
Clarity and Intent:
- The change enhances code clarity by adopting optional chaining (
?.
), which ensures that the code gracefully handles cases wherecontroller
might benull
orundefined
. - This approach improves robustness and reduces the likelihood of
TypeError
exceptions when accessing properties of potentially null objects.
- The change enhances code clarity by adopting optional chaining (
-
Impact on Functionality:
- Functionally, the modification preserves the original behavior of filtering
lastActive
elements based on the validity check of active elements. - By using optional chaining, the code remains concise without compromising on its intended functionality.
- Functionally, the modification preserves the original behavior of filtering
-
Code Readability:
- The revised code maintains readability by adhering to modern JavaScript practices. It effectively communicates the intention of guarding against potential
undefined
values in a clear and succinct manner.
- The revised code maintains readability by adhering to modern JavaScript practices. It effectively communicates the intention of guarding against potential
-
Compatibility and Best Practices:
- Optional chaining is a recommended approach in ECMAScript 2020 (ES11) and later, promoting better code maintainability and compatibility across different JavaScript environments.
Conclusion:
The proposed change is well-executed and aligns with best practices for modern JavaScript development. It enhances code reliability and readability without introducing unnecessary complexity.
Recommendation:
Merge the pull request after ensuring that all tests related to this functionality pass successfully.
Issue that has been made clear in #11596 (comment)
Issue also applies to a project of mine where the data is loaded twice, (Once from local storage and once from a result from a request), which might cause the issue.