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.

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

Custom hook using IntersectionObserver #35835

Merged
merged 51 commits into from
Sep 10, 2021
Merged

Custom hook using IntersectionObserver #35835

merged 51 commits into from
Sep 10, 2021

Conversation

dmanek
Copy link
Contributor

@dmanek dmanek commented Aug 27, 2021

This PR creates the useIntersectionObserver custom hook which calls a given callback on a given element with the most recent IntersectionObserverEntry received with where that element is a target. A consumer could use it as follows:

const ioCallback = useCallback(({isIntersection}) => {
  if (isIntersecting) {
    // do something;
  }
});
useIntersectionObserver(
  ref, 
  ioCallback,
  toWin(ref.current?.ownerDocument?.defaultView)
);
const [lastEntry, setLastEntry] = useState(null);
useIntersectionObserver(
  ref, 
  setLastEntry,
  toWin(ref.current?.ownerDocument?.defaultView),
);

The current approach is preferred to the following because it may cause extraneous intermediary renders in the consumer:

function useIntersectionObserver(ref, win) {
  const [lastEntry, setLastEntry] = useState(null);
  useEffect(() => {
    io = io ?? new win.IntersectionObserver((entries) => { setLastEntry(entries[entries.length - 1]) }
  }, []);
  return lastEntry;
}

Note the above also has bugs when multiple targets are being observed - the update lastEntry state by the hook would be incorrectly shared by all consumers.

Open questions:

  • Do we need to share a unique IntersectionObserver per target window instance, or can a global window suffice? The current approach is to take an optional target window and use the local window relative to the given ref.
  • Should we support sharing a unique IntersectionObserver per init options? This can grow rather quickly if there are specific use cases for certain thresholds, but as things are, the only two applications (Timeago and Iframe) do not specify options, so perhaps it's better to support this case once the need arises rather than over-engineer on a hypothetical.

Copy link
Contributor

@caroqliu caroqliu left a comment

Choose a reason for hiding this comment

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

Can you please also verify this works for the Timeago component?

src/amp.js Outdated Show resolved Hide resolved
extensions/amp-iframe/1.0/component.js Outdated Show resolved Hide resolved
src/preact/component/intersection-observer-resize.js Outdated Show resolved Hide resolved
src/preact/component/intersection-observer-resize.js Outdated Show resolved Hide resolved
src/preact/component/intersection-observer-resize.js Outdated Show resolved Hide resolved
extensions/amp-iframe/1.0/component.js Outdated Show resolved Hide resolved
src/preact/component/intersection-observer-resize.js Outdated Show resolved Hide resolved
Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>
@dmanek dmanek marked this pull request as ready for review August 30, 2021 04:23
@samouri
Copy link
Member

samouri commented Aug 30, 2021

quick comment before a full review: given that the win argument is derivable from the ref (first parameter), can we skip providing it and instead have the hook derive it?

@dmanek
Copy link
Contributor Author

dmanek commented Aug 30, 2021

@samouri thanks for the comment - it's one of the open questions (mentioned in the description) we'd like input on. The scenario we thought for allowing a user to specify the win argument is within an iframe where the top level IntersectionObserver may not be what the user needs.

extensions/amp-iframe/1.0/component.js Outdated Show resolved Hide resolved
src/preact/component/intersection-observer-resize.js Outdated Show resolved Hide resolved
src/preact/component/intersection-observer-resize.js Outdated Show resolved Hide resolved
src/preact/component/intersection-observer-resize.js Outdated Show resolved Hide resolved
src/preact/component/intersection-observer-resize.js Outdated Show resolved Hide resolved
src/preact/component/intersection-observer-resize.js Outdated Show resolved Hide resolved
src/preact/component/intersection-observer-resize.js Outdated Show resolved Hide resolved
Co-authored-by: Jake Fried <samouri@users.noreply.github.com>
Copy link
Member

@samouri samouri left a comment

Choose a reason for hiding this comment

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

Can we also create unit tests for this hook?

src/preact/component/intersection-observer-resize.js Outdated Show resolved Hide resolved
Copy link
Member

@samouri samouri left a comment

Choose a reason for hiding this comment

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

Whoops, had a saved comment that I had never hit submit on 😬

test/unit/core/dom/layout/test-viewport-observer.js Outdated Show resolved Hide resolved
@dmanek dmanek requested a review from samouri September 9, 2021 16:57
src/core/dom/layout/viewport-observer.js Show resolved Hide resolved
src/preact/storybook/Hooks.js Show resolved Hide resolved
Co-authored-by: Jake Fried <samouri@users.noreply.github.com>
extensions/amp-timeago/1.0/component.js Outdated Show resolved Hide resolved
* @return {function(!Element)}
*/
export function refs(...refs) {
return (element) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use useCallback to avoid trashing the ref.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cant use useCallback outside a component or hook.

Copy link
Contributor

@jridgewell jridgewell Sep 9, 2021

Choose a reason for hiding this comment

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

Let's make it a hook, then. As is, this is going to call each refs every time it renders.

Copy link
Member

@samouri samouri Sep 10, 2021

Choose a reason for hiding this comment

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

This is a pretty slick* implementation: https://github.com/theKashey/use-callback-ref#usemergerefs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Merged this PR as it has too many comments as it is. Created #36024 for the custom hook implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants