-
Notifications
You must be signed in to change notification settings - Fork 34
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
π Tracking errors on the new dashboard tab #986
Comments
Also reproducible on prod JS, this time on a usaid-laos opcode with no trips. Not producible on dev JS or any other codes I tried.
|
For the ChartJS is tree-shakable and it accomplishes this by having each element be individually registered. So you only import the elements you actually need. We import the ones we need at https://github.com/e-mission/e-mission-phone/blob/87b3456cce53ebdf3bebfaf88332ef8270adda16/www/js/components/Chart.tsx#L10. I added a debugger breakpoint on the above line to make sure it was actually being reached. It is, but we get an error anyway. |
Plenty of people online have struggled with this error because they didn't properly register the required Chart elements. https://stackoverflow.com/questions/67060070/chart-js-core-js6162-error-error-line-is-not-a-registered-controller/67143648#67143648 We would miss out on tree-shaking but it might be our only option. |
Well However, I tried the other option from the above stackoverflow thread, which was to import all That worked. So I think that's what we're going to go with right now. And then I'll also file an issue on the ChartJS repo, hopefully to get resolved eventually. |
Unfortunately, this does mean that in the meantime we have to carry the entire bundle size of ChartJS (which I'm frankly not sure how big it is) |
I haven't been able to reproduce the |
Described in e-mission/e-mission-docs#986 (comment), we have a production-specific error where the needed ChartJS elements are not recognized as being registered. Likely an incompatibility between ChartJS and Webpack. As a workaround, we will register all of the `registerables`. Unfortunately, we will no longer benefit from tree-shaking with this approach.
I think we should just get #1039 merged so we can continue testing on staging. |
Gah! That's pretty bad. However, if we make the app more visual, maybe we will end up using more of the charts anyway. |
@JGreenlee @shankari I'm not sure if this relates to earlier errors, but I am experiencing an error with the dashboard with my Simulator.Screen.Recording.-.iPhone.13.Pro.-.2023-11-07.at.09.57.20.mp4The error I see in the debugger is To add, this is not happening on any of my test phones, so maybe it is specific to the opcode? |
With that same opcode on both my personal phone and in the emulator this morning the dashboard is working fine, no WSOD. |
I wonder if #1025 is a dup of this. If you see the video there, the label screen flashes for a second before going to WSOD. |
This may help with debugging e-mission/e-mission-docs#986 (comment)
I don't think it is the same issue, but I did also include some logs in e-mission/e-mission-phone#1100 that might help us debug this one. |
The issue is in So this seems like a sneaky regression caused by the interaction of 2 PRs that were merged independently |
why is it intermittent, though? |
This is a different issue altogether. I think the dashboard has gotten pretty messed up with all the rewrites and it is going to need a batch of bugfixes before the next release |
I just logged in to my |
Reproducible on prod JS with my personal nrel-commute OPCode on both phone + devapp. Not reproducible on dev JS.
The text was updated successfully, but these errors were encountered: