LPS-116140 Custom event listeners in openModal util are initialized multiple times #59
Conversation
|
To conserve resources, the PR Tester does not automatically run for every pull. If your code changes were already tested in another pull, reference that pull in this pull so the test results can be analyzed. If your pull was never tested, comment "ci:test" to run the PR Tester for this pull. |
|
ci:test:sf |
|
ci:test:relevant |
liferay-continuous-integration
added
ci:test:sf - pending
ci:test:sf - success
and removed
ci:test:sf - pending
labels
✔️ ci:test:sf - 1 out of 1 jobs passed in 3 minutesClick here for more details.Base Branch:Branch Name: master Sender Branch:Branch Name: LPS-116140 1 Successful Jobs:For more details click here. |
I'd strongly advise against that. The rule is provided by the React team for a reason:
(And I know you've already seen this but I am going to reshare it here anyway...) This issue goes into immense detail about the kinds of "false positives" one may see with this rule (and solutions), as well as real problems and what it is that makes them so; specifically, this summary comment. Overall, my feeling is that this rule is sometimes annoying, but overall helps us more than it hurts us. |
| selectEventName, | ||
| ]); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, []); |
There was a problem hiding this comment.
Would be interesting to know which item(s) in the deps array was/were triggering unwanted updates. One that stands out is this one, customEvents, which has a default value, so if not passed, you'll get a different array every time the function is called. A safer default for that would be null, and you'd just need an if (customEvents) guard before using it. This in general is true with React components, that non-primitive defaults (common examples being functions, objects, arrays) can be a source of trouble for exactly this reason.
If you wanted to find out which elements in the array are the cause, you could do a comparison with Object.is(), which is what React is using internally to perform these checks.
In my experience I believe refs tend to be pretty stable, but I don't think there are any guarantees documented anywhere. I'd be interested to hear from anybody who has "caught" useRef returning an unstable value; at least playing around in CodeSandbox now I always get back the same thing in a trivial example. I remember looking in the React source some months back to figure that out, but I've forgotten so would need to look again...
The thing we actually care about is eventHandlersRef.current anyway; as you know, if you stick that in the array, the lint rule will tell you:
React Hook React.useEffect has an unnecessary dependency: 'ref.current'. Either exclude it or remove the dependency array. Mutable values like 'ref.current' aren't valid dependencies because mutating them doesn't re-render the component.
FWIW, if there is any evidence that the ref is changing, I think I would just omit it from the list and if the linter complains, suppress it.
But looking at the React source, I'd say this shows the ref to be memoized and stable, so it is very unlikely to be the problem, and it seems safe for it to stay in the array.
There was a problem hiding this comment.
I think it was visible, based on my analysis at #50 (comment)
There was a problem hiding this comment.
Would be interesting to know which item(s) in the deps array was/were triggering unwanted updates.
The ones causing the unwanted updates are customEvents and processClose. You are right, the ref is safe.
One that stands out is this one, customEvents, which has a default value, so if not passed, you'll get a different array every time the function is called.
Removing default for customEvents and wrapping usage in if did work! That's such a weird quirk, I would never have guessed it works that way 😮
That leaves processClose. It's a hard nut to crack, we are changing state in it. Is there some magic to workaround that one?
If not, does it make sense to keep partial list of arguments? We would still have to keep the // eslint-disable-next-line react-hooks/exhaustive-deps line.
There was a problem hiding this comment.
That leaves
processClose. It's a hard nut to crack, we are changing state in it. Is there some magic to workaround that one?
As it turns out, reverting useCallback does the job.
With this, we don't need the rule exception. Let me push the changes.
There was a problem hiding this comment.
I think it was
visible, based on my analysis at #50 (comment)
visible as an argument indeed would cause updates. That's why I removed it, by reverting the commit where you added it.
There was a problem hiding this comment.
visible as an argument indeed would cause updates. That's why I removed it, by reverting the commit where you added it.
😂
Didn't realize I added it!!! This lint rule is waaaay toooo smart!! 😱
There was a problem hiding this comment.
This lint rule is waaaay toooo smart!!
The ones causing the unwanted updates are
customEventsandprocessClose. You are right, the ref is safe.
But processClose only because you got rid of the useCallback, right? (I haven't looked at your new changes yet, so I don't know if you added back the useCallback.)
... and reading further down the thread I see you discovered that.
That leaves
processClose. It's a hard nut to crack, we are changing state in it. Is there some magic to workaround that one?
I feel like we discussed useCallback on that one in a prior iteration, but I can't find the reference to it. 🤷
There was a problem hiding this comment.
I feel like we discussed
useCallbackon that one in a prior iteration, but I can't find the reference to it. 🤷
This was it: https://github.com/wincent/liferay-portal/pull/295#discussion_r430402239
|
Just to complement discussions about |
…ery time the function is called. Preventing this on `customEvents` means we can maintain explicit list of dependencies.
|
ci:test:sf |
|
ci:test:relevant |
✔️ ci:test:sf - 1 out of 1 jobs passed in 3 minutesClick here for more details.Base Branch:Branch Name: master Sender Branch:Branch Name: LPS-116140 1 Successful Jobs:For more details click here. |
|
Since this is green and you indeed removed my ugly workaround... let's see how this plays out :) |
|
ci:forward |
|
CI is automatically triggering the following test suites:
The pull request will automatically be forwarded to the user
|
|
Skipping previously passed test suites: |
|
All required test suite(s) passed. |
|
Pull request has been successfully forwarded to brianchandotcom#90654 |
@wincent @jbalsas
In #11 and #50, we noticed an issue with multiplication of custom events execution in modals. The workaround was brianchandotcom@df771f0. In this PR we are reverting the workaround and fixing the root cause of the issue.
To reproduce the issue:
That type is now added twice to the main page. If modal is opened x times, type will be added x times.
The root cause is that
useEffectis triggered multiple times. It is suppose to trigger only once on mount, but it triggers three times. It behaves like the component is unmounted and mounted multiple times. This cancels out so we are not noticing this issue everywhere, but it also triggers multiple times on close, where it shouldn't, and that causes the noticeable issue. The standard form for hook-based code we want to execute on mount and unmount is to have and empty array as an argument ofuseEffect:This clashes with our source formatter, where anything used in
useEffectmust be added as an argument. So in this PR, we are adding an exception. We didn't do this before because we were assuming that arguments with constant values can be safely used. This is true for prop strings, likeselectEventNameandid, but, as it turns out, is not true for functions and refs. Any change in them causes change inuseEffect. For example, in our case, inonClosewe changevisiblestate, and that triggersuseEffectupdate.@wincent I know we had this issue before, it may be a good idea to drop the
react-hooks/exhaustive-depsrule.Now, there is a separate issue: we have a small memory leak when we close modal and reopen it. It comes from the fact that we don't unmount component on close, and create new component on open. This doesn't cause the issue in this task, because we are explicitly cleaning up event handlers on close.
It is a small memory leak, as the components stay alive only until user navigates to a different page. Then, unmount handlers of all components execute. Note - nothing really happens in those unmount handlers: we are cleaning up event handlers, but they have all been previously cleaned in close event. This is more of a security cleanup, for some future unanticipated edge case.
I am not sure that this can be fixed, without changing how we integrate React components into Liferay portal. In React, I don't think there is a way to explicitly unmount a component. What we would do is to propagate
visibleto the parent component and havevisible && <Modal>in it. But, by usingrender(Modal), we don't have a parent component. I also don't see a way to make any kind of a singleton pattern, it would have to be something on a lever higher thanModalcomponent, maybe in therenderimplementation?Ultimately, I feel this issue is minor, I don't think there is any reasonable use case where modal is opened and closed so many times that the increased memory usage becomes noticeable. If fixing this would require major restructuring, I don't think it is worth it.