Skip to content
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.

Sign up for GitHub

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

LPS-116140 Custom event listeners in openModal util are initialized multiple times #59

Closed
wants to merge 3 commits into from
Closed

LPS-116140 Custom event listeners in openModal util are initialized multiple times #59

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b397c78
Revert "LPS-113561 Avoids adding event handlers when the modal is not…
markocikos Jun 25, 2020
d85d190
LPS-116140 Prevent multiple event listener initialization
markocikos Jun 25, 2020
f035e96
LPS-116140 If a prop has a default value, we get a different array ev…
markocikos Jun 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 33 additions & 47 deletions .../frontend-js/frontend-js-web/src/main/resources/META-INF/resources/liferay/modal/Modal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import classNames from 'classnames';
import {render} from 'frontend-js-react-web';
import dom from 'metal-dom';
import PropTypes from 'prop-types';
import React, {useCallback, useEffect, useRef, useState} from 'react';
import React, {useEffect, useRef, useState} from 'react';

import './Modal.scss';
import navigate from '../util/navigate.es';
Expand Down Expand Up @@ -183,7 +183,7 @@ const Modal = ({
}
};

const processClose = useCallback(() => {
const processClose = () => {
setVisible(false);

document.body.classList.remove('modal-open');
Expand All @@ -199,7 +199,7 @@ const Modal = ({
if (onClose) {
onClose();
}
}, [eventHandlersRef, onClose]);
};

const Body = ({html}) => {
const bodyRef = useRef();
Expand All @@ -226,47 +226,42 @@ const Modal = ({
useEffect(() => {
const eventHandlers = eventHandlersRef.current;

if (visible) {
if (onSelect && selectEventName) {
const selectEventHandler = Liferay.once(
selectEventName,
(selectedItem) => {
processClose();
if (onSelect && selectEventName) {
const selectEventHandler = Liferay.on(
selectEventName,
(selectedItem) => {
processClose();

onSelect(selectedItem);
}
);
onSelect(selectedItem);
}
);

eventHandlers.push(selectEventHandler);
}
eventHandlers.push(selectEventHandler);
}

customEvents.forEach((customEvent) => {
if (customEvent.name && customEvent.onEvent) {
const eventHandler = Liferay.on(
customEvent.name,
(event) => {
customEvent.onEvent(event);
}
);
customEvents.forEach((customEvent) => {
if (customEvent.name && customEvent.onEvent) {
const eventHandler = Liferay.on(customEvent.name, (event) => {
customEvent.onEvent(event);
});

eventHandlers.push(eventHandler);
}
});
eventHandlers.push(eventHandler);
}
});

const closeEventHandler = Liferay.on('closeModal', (event) => {
if (event.id && id && event.id !== id) {
return;
}
const closeEventHandler = Liferay.on('closeModal', (event) => {
if (event.id && id && event.id !== id) {
return;
}

processClose();
processClose();

if (event.redirect) {
navigate(event.redirect);
}
});
if (event.redirect) {
navigate(event.redirect);
}
});

eventHandlers.push(closeEventHandler);
}
eventHandlers.push(closeEventHandler);

return () => {
eventHandlers.forEach((eventHandler) => {
Expand All @@ -275,17 +270,8 @@ const Modal = ({

eventHandlers.splice(0, eventHandlers.length);
};
}, [
customEvents,
eventHandlersRef,
id,
onClose,
onOpen,
onSelect,
processClose,
selectEventName,
visible,
]);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@jbalsas jbalsas Jun 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it was visible, based on my analysis at #50 (comment)

Copy link
Collaborator Author

@markocikos markocikos Jun 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!! 😱

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This lint rule is waaaay toooo smart!!

smart

The ones causing the unwanted updates are customEvents and processClose. 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. 🤷

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like we discussed useCallback on 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


return (
<>
Expand Down