Repository navigation
Fix: recreate handlers and reset completed state on expression change - #33900
Conversation
cause the component to re-render when these values change
use connectAdvanced instead of connect since it provides a way to get a hold of dispatch just one time, so the handlers object can be built in the container and only updated when something actually changes
allow completeFn to be called again, required so that the correct external actions happen
c68e381 to
96481e2
Compare
💚 Build Succeeded |
💚 Build Succeeded |
💚 Build Succeeded |
monfera
left a comment
There was a problem hiding this comment.
It might need some cleanup, it'll stop on an error if, starting with the reproduction sequence in #33876
- set expression to
timefilterControl compact=true column=@timestamp | render - set expression to
demodata - then set it back to
timefilterControl compact=true column=@timestamp | render
renderFn threw Error: Objects must have a type property
at getType (get_type.js:39)
at Array.<anonymous> (ast.js:114)
at Array.map (<anonymous>)
at getExpression (ast.js:113)
at toExpression (ast.js:158)
at render (index.js:39)
at RenderWithFn._this.callRenderFn (render_with_fn.js:66)
at RenderWithFn.componentDidUpdate (render_with_fn.js:138)
at commitLifeCycles (react-dom.development.js:17143)
at commitAllLifeCycles (react-dom.development.js:18530)
Also, if it's not a burden, pls. add a small comment line (for anyone else who might want to review) where it's possible to observe the correct new values of handlers.getFilter() where it's set (it's easy to find where handlers.getFilter() values are used).
Slightly related: I wonder if there's potential for some code DRY-up (likely not in this PR but eventually), the way handlers.getFilter() is used is done twice (via fromExpression) - probably the answer is no, but perhaps handlers.getAst() could be an alternative? Ie. eagerly run the expression through fromExpression and then there's no reason to run fromExpression on subsequent uses.
rebuild on happens when id, filter, or expression change
8f6db7f to
561ddab
Compare
@monfera I believe this is actually another bug, and one that existed already. When you switch to demodata, the filter value gets cleared. When you then switch back to timefilterControl, the filter is not empty, so when it tries to build the ast from it at render time, it fails. Though, usually the error messgae you see is different, so it's possible there's something else going on here. I'll look into it, thanks for the feedback. UPDATE: When I do these steps, I get this, which is the result of what I just explained: I don't know how to get into the error state you described. UPDATE 2: Figured it out, it's the same bug, but I get your error condition if I merge #28796 into my branch. I think you just didn't rebuild your interpreter before trying this out. This leads me to believe there's a bug in the AST translation in that PR now, so I'll check into that. UPDATE 3: Found the cause, and left a comment with details on the empty expression PR. Also, the issue to track the invalid filter expression, which is the real cause of all of this, is here: #33605 |
💚 Build Succeeded |
|
LGTM, as well. Nit: I'd like to start seeing some of these files in PRs be converted to TypeScript, even if there are |
|
Since I see this is being backported to 6.7, that's a definite exception to my TS nit. Carry on. :-) |
|
@monfera @clintandrewhall Still looking for feedback on this too:
To further clarify, the edge case is if you have an existing element and you change from a filter function to a non-filter function, or vice versa. It seems somewhat uncommon that users would do this, but it leaves them in a really bad state if they do. |
|
Due to the positive impact of the change - solution for a likely rare but rather impactful bug - and other considerations, such as the longevity of |
|
I'm in favor of backporting to 6.7. In a weird way, the fact it's an edge case means that it likely won't endanger the release, but it will prevent bug reports coming in from a known issue, (and having to say, upgrade to 7.0) |
…#33900) (#34481) * fix: add expression and filter to ElementWrapper propso cause the component to re-render when these values change * fix: correctly spread additional props * chore: convert ElementWrapper to functional component * chore: refactor ElementWrapper container use connectAdvanced instead of connect since it provides a way to get a hold of dispatch just one time, so the handlers object can be built in the container and only updated when something actually changes * fix: reset handlers isComplete when element changes allow completeFn to be called again, required so that the correct external actions happen * feat: make expression available on shapes object * fix: reset done checker on function change * fix: only rebuild handlers when element changes rebuild on happens when id, filter, or expression change * chore: remove unused ElementWrapper props
…#33900) (#34482) * fix: add expression and filter to ElementWrapper propso cause the component to re-render when these values change * fix: correctly spread additional props * chore: convert ElementWrapper to functional component * chore: refactor ElementWrapper container use connectAdvanced instead of connect since it provides a way to get a hold of dispatch just one time, so the handlers object can be built in the container and only updated when something actually changes * fix: reset handlers isComplete when element changes allow completeFn to be called again, required so that the correct external actions happen * feat: make expression available on shapes object * fix: reset done checker on function change * fix: only rebuild handlers when element changes rebuild on happens when id, filter, or expression change * chore: remove unused ElementWrapper props
…elastic#33900) * fix: add expression and filter to ElementWrapper propso cause the component to re-render when these values change * fix: correctly spread additional props * chore: convert ElementWrapper to functional component * chore: refactor ElementWrapper container use connectAdvanced instead of connect since it provides a way to get a hold of dispatch just one time, so the handlers object can be built in the container and only updated when something actually changes * fix: reset handlers isComplete when element changes allow completeFn to be called again, required so that the correct external actions happen * feat: make expression available on shapes object * fix: reset done checker on function change * fix: only rebuild handlers when element changes rebuild on happens when id, filter, or expression change * chore: remove unused ElementWrapper props
Summary
Closes #33876
Closes #33898
I tried to do this as two PR's, but the two issues were related, and the fixes were dependent on one another.
When an Element's expression changes, re-create the handlers object and clear the internal
isCompletedflag.handlersmethods (getFilterin particular) return the correct valuehandlers.done()correctly calls theonCompletemethodUpdating the handlers object
Required so that
handlers.getFilteralways returns the current value. The handlers object is stateful, so it can't simply be reused.To do this, I converted the redux stuff in the container using connectAdvanced, which allows the function that creates that object to get the
dispatchmethod just one time. Then a stripped downelementsobject is added to the produced props, and the new props object is deeply compared to the proposed new props object; if nothing changes, the old object is still used.This provides the same functionality as #31734, allowing the removal of the
shouldUpdateHoC. Since the props object will be pure unless there's really some change, the element does not re-render unless it needs to.Additionally, withPropsOnChange is used to re-create the handlers object only when the element object changes, and in particular, the id, filter, or expression.
Finally, mapProps is used to remove props not used in the component.
elementsandcreateHandlersare only used to build the handlers object, as isselectedPage. I also removed theerrorprop, which was unused but had nothing else to do with the changes here.mapPropsonly gets run if the props object changes, so the handlers object is now pure, and theElementWrappercomponent itself could be converted back to a functional component.ElementWrapperdoesn't re-render unless it needs toClearing of the
isCompletestate on the handlers objectSince the
createHandlersfunction is now two functions, where the first just allows partial application of thedispatchfunction, it also allowed changing the internal state based on theelementobject. That object now includes the expression, in addition to the filter that was already there, and when it changes,isCompleteis reset. This allows future calls tohandlers.doneto call theonChangehandler when the element changes.Additionally, the
ElementShareContainer, which is responsible for tracking whether or nothandlers.donewas called from a specific render function, was modified to clear and re-create the check when the render function changes. This is what fixes #33898.handlers.donealso correctly callsonCompleteTesting
In the case of #33876, you'll need to modify the
tablerender function toconsole.log(handlers.getFilter())(or otherwise show you the value at render time). Do this before pulling down this PR to confirm that it doesn't work correctly, and after pulling down this PR to confirm that it now does.As for #33898, the steps there require no local changes.
Open Question
Should this be backported to 6.7? My thought it yes,
handlers.getFilterreturning the wrong value leads to really messed up filter states. But it's also kind of an edge case.