-
Notifications
You must be signed in to change notification settings - Fork 362
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
Pathopt broken. Looks like across unions #876
Comments
Hrm I wasn't aware that pathopt regressed. If we can keep it that's preferred since it was done to address a performance issue in more complex scenarios. That is of course unless people have evidence that there are better ways to get perf benefits? |
Well, Untangled has used it since the beginning, but I had to recently set it to off because of these recent issues. Until yesterday I wasn't sure what was causing the problems, but I suspected pathopt. I've now got two verified cases where it causes failure (where I can flip it off/on and see the effect). One happens when transacting at some UI depth where a pathopt refresh can happen. This one is a mismatch between how the component is indexed and how the lookup is attempted in the indexer. I get errors on "no queries exist" at a UI component path that includes an ident. The other is similar, and happens because of the assumption here: Line 2534 in 6c3da39
that the child path will always be longer than the child. Unfortunately, sometimes the om-path of the child starts at an ident. This causes OOB exceptions. Since it's been on by default in Untangled, I suspect my users have been seeing issues with it for some time that they've been working around by moving transacts closer to the root. |
@awkay that's helpful. I don't have time to look at it today, probably tomorrow but I'd rather just see this get sorted out. |
@awkay I'm happy to look at a minimal failing case and figure it out |
@anmonteiro I'm on the road at the moment (for another week or so) visiting family so I have limited time. I'll see if I can put one together. If you look at the line I referenced above, you should see the obvious potential problem when a child's om-path is something like I'm not sure if the indexer problem is it trying to look up something via an ident-path, or storing something with an ident-path but trying to look it up with a root path. I suspect the former. Perhaps I'll try to put together the two failing cases soon. |
So, I'm trying to reproduce the issue with Om Next without Untangled and I'm having trouble making it misbehave. I'm not sure I'm doing the right things in Om Next to trigger a child re-render with pathopt (it looks like I am), but I'm never seeing the om path become rooted at an ident. Sleepy, so that isn't helping. I've even tried triggering an update through a simulated remote. I cannot figure out how om-path is getting hosed in my larger program, so I'm probably going to have to more carefully trace one of the larger programs that I have in order to better isolate the problem. |
OK, I copied code out of the untangled project that was failing and tried to strip non-essentials. The CSS doesn't quite work right in the devcard, but you'll be able to use it. https://github.com/awkay/pathopt-issue Instructions:
Now edit the code, and change pathopt (line 213) to false. It works fine. I tried writing a similar smaller thing but I could not get it to fail, so there is some chance that there is an error on my part, but I don't see it. |
So, I removed all other deps (including devcards). The instructions change to (2) being just open the index.html file. The rest continues to be true (sorry, the CSS is still off). I thought about the possible problem and tried to narrow the condition under which it fails, so I added a couple of child buttons to try the following:
So, I added two buttons (on the other-child branch), one that disappears when its click count is even. (the other button is so you can increment the click count). Both work fine with pathopt on. The nav menu, however, still fails with pathopt on after a patopt re-render. (it starts failing to find the queries). |
I figured it out as I was porting the internals of Om next to Fulcro 2.0. It's a nefarious little thing, but the fix is easy. See: |
I've recently had multiple problems when using pathopt (which Untangled has had on by default from the early days). Most recently, the changes in alpha42 to the reconciler (OM-738) make assumptions that the path info on the parent will be shorter than the path info on the child (which is not true when the path ends up rooted at the child's ident). This causes Index OOB exceptions.
I've also seen pathopt lead to "no queries exist for components at path" where the path was an ident-based path. I'm sure this is a similar problem.
I've changed Untangled to turn if off by default, and that clears up the issues.
The question is: should we fix pathopt, or remove the vestiges of it?
The text was updated successfully, but these errors were encountered: