Год назад моя компания впервые попросила меня провести собеседование для фронтендера. Тогда я и придумал эту задачу на свою злобу дня. Задачка простая, на базовые знания, но, как оказалось, в ней можно сделать много интересных ошибок. Также в ней оказались неочевидные детали, которые могли бы оказаться интересны и опытным разработчикам. Я решил достаточно подробно разобрать задачу в этом посте в педагогических целях. Данный пост нацелен в первую очередь на начинающих реакт разработчиков.

Формулировка

Есть реализация компонента, от которого требуется 2 вещи:

1) выводить текущее значение вертикального скролла окна (window.scrollY)

2) после монтирования асинхронно получить число и вывести его

Нужно найти, объяснить и исправить как можно больше проблем в реализации

import React, { useState, useEffect } from 'react';

// имитация запроса к серверу. просто получаем число асинхронно
const fetchRandomNumber = () => Promise.resolve(Math.random());

const NumberAndScroll = () => {
  const [number, setNumber] = useState();
  const [scroll, setScroll] = useState();
  
  useEffect(async () => {
    setNumber(await fetchRandomNumber());
    
    window.addEventListener('scroll', () => setScroll(window.scrollY));

    return () => window.removeEventListener('scroll', () => setScroll(window.scrollY));
  });
  
  return (
    <div>
      <div> Number: { number } </div>
      <div> Scroll: { scroll } </div>
    </div>  
  )
}

Настоятельно рекомендую самостоятельно исправить всё что найдёте, прежде чем читать дальше.

Большие проблемы

Первое, что все правильно замечают - отсутствие второго аргумента у useEffect. Там должен быть пустой массив зависимостей, потому что число нам нужно получить с "сервера" только 1 раз при монтировании. Да и неоднократно переподписываться на событие скролла не имеет смысла.

import React, { useState, useEffect } from 'react';

const fetchRandomNumber = () => Promise.resolve(Math.random());

const NumberAndScroll = () => {
	const [number, setNumber] = useState();
  const [scroll, setScroll] = useState();
  
  useEffect(async () => {
    setNumber(await fetchRandomNumber());
    
    window.addEventListener('scroll', () => setScroll(window.scrollY));
    
    return () => window.removeEventListener('scroll', () => setScroll(window.scrollY));
  }, []); // добавили вторым аргументом массив зависимостей
  
  return (
  	<div>
      <div> Number: { number } </div>
      <div> Scroll: { scroll } </div>
    </div>	
  )
}

Частой ошибкой бывает передача такого массива зависимостей: [scroll]. Если вы тоже совершили эту ошибку, то вернитесь к коду и убедитесь, что нам не нужно текущее значение скролла, чтобы обновлять его.

Второе, на что обычно обращают внимание, это ключевое слово async у колбека. Многие имели свой личный опыт писать так и знают, что так делать нельзя. Но почему? Этот вопрос ставит некоторых в тупик. Согласно документации реакта, колбек должен либо ничего не возвращать (читай возвращать undefined) либо возвращать функцию очистки. То есть только 2 валидных значения есть для возврата: undefined и функция. Но вы же знали, что функции объявленные async всегда возвращают промис? То есть в текущей реализации колбек возвращает не функцию очистки, а промис этой функции. Нужно убрать ключевое слово async и переделать колбек, чтобы работало. С этим тоже бывали проблемы у новичков. Самое простое - написать через промис:

import React, { useState, useEffect } from 'react';

const fetchRandomNumber = () => Promise.resolve(Math.random());

const NumberAndScroll = () => {
 	const [number, setNumber] = useState();
  const [scroll, setScroll] = useState();
 
  // убрали async
  useEffect(() => {
    // переписали через промис
    fetchRandomNumber().then(setNumber);
    
    window.addEventListener('scroll', () => setScroll(window.scrollY));
    
    return () => window.removeEventListener('scroll', () => setScroll(window.scrollY));
  }, []);
  
  return (
  	<div>
      <div> Number: { number } </div>
      <div> Scroll: { scroll } </div>
    </div>	
  )
}

Если вы написали fetchRandomNumber().then(num => setNumber(num)), то уделите минуту, чтобы осознать, что fetchRandomNumber().then(setNumber) делает то же самое, только короче и оптимальнее.

Последняя грубая ошибка в том, что очистка листенера всё равно не происходит. Я попытаюсь привести свою аналогию для объяснения ошибки. Допустим вы купили новый телефон. Вы ставите будильник на нём на 5 утра, чтобы встать для пробежки по набережной. А потом вы поняли, что бег это дичь и передумали. Но вместо того, чтобы выключить будильник на своём новом телефоне, вы идёте снова в магазин, покупаете ещё один телефон той же самой модели, открываете в нём будильники и выключаете все на 5 утра (впрочем там и выключать нечего). Оба телефона абсолютно идентичны и видимых различий практически нет, но это всё же разные телефоны. В реализованном коде написана примерно такая же бессмыслица. Каждый раз, когда мы пишем выражение () => setScroll(window.scrollY), мы создаём новую функцию. Часто это проблема решалась несколько странно:

import React, { useState, useEffect } from 'react';

const fetchRandomNumber = () => Promise.resolve(Math.random());

const NumberAndScroll = () => {
  const [number, setNumber] = useState();
  const [scroll, setScroll] = useState();
  
  // вынесли создание функции сюда
  const handleScroll = () => setScroll(window.scrollY);
  
  useEffect(() => {
    fetchRandomNumber().then(setNumber);
    
    window.addEventListener('scroll', handleScroll);
    
    return () => window.removeEventListener('scroll', handleScroll);
  }, []);
  
  return (
    <div>
      <div> Number: { number } </div>
      <div> Scroll: { scroll } </div>
    </div>    
  )
}

Это работает, но проблема тут в том, что handleScroll создаётся на каждый рендер компонента, а используется только в колбеке useEffect'а. То есть только при первом рендере. То есть выполняется лишняя работа и код более разбросан. Правильней так:

import React, { useState, useEffect } from 'react';

const fetchRandomNumber = () => Promise.resolve(Math.random());  

const NumberAndScroll = () => {      
  const [number, setNumber] = useState();   
  const [scroll, setScroll] = useState();        
  
  useEffect(() => {     
    fetchRandomNumber().then(setNumber);      
    
    // перенес создание в useEffect
    const handleScroll = () => setScroll(window.scrollY);
    
    window.addEventListener('scroll', handleScroll);   
    
    return () => window.removeEventListener('scroll', handleScroll);
  }, []);  
  
  return (      
    <div>       
      <div> Number: { number } </div> 
      <div> Scroll: { scroll } </div>    
    </div>  
)}

Менее важные проблемы

Ещё один момент, на который я прошу обратить внимание кандидатов - начальные значения состояний. Новички часто пишут так:

const [number, setNumber] = useState(0);
const [scroll, setScroll] = useState(0);

Есть люди, которым кажется, что начальное значение всегда обязательно должно быть задано. И если это число, то наверняка 0. Но в этой задаче мы не можем дать переменной number адекватного начального значения и лучше оставить его undefined. Для переменной scroll начальное значение 0 неверно, потому что наш компонент может появиться когда страничка браузера уже прокручена вниз. Ожидаемый код такой:

import React, { useState, useEffect } from 'react';

const fetchRandomNumber = () => Promise.resolve(Math.random());

const NumberAndScroll = () => {
 	const [number, setNumber] = useState();
  // добавил начальное значение
  const [scroll, setScroll] = useState(window.scrollY); 
  
  useEffect(() => {
    fetchRandomNumber().then(setNumber);

	  const handleScroll = () => setScroll(window.scrollY);
    
    window.addEventListener('scroll', handleScroll);
    
    return () => window.removeEventListener('scroll', handleScroll);
  }, []);
  
  return (
  	<div>
      <div> Number: { number } </div>
      <div> Scroll: { scroll } </div>
    </div>	
  )
}

Вот уже более или менее приличный код вышел. На этом этапе я обычно перехожу к следующей задаче в собеседовании. Но если кандидат бодро справляется, я предлагаю попробовать ещё как-то улучшить код.

Плюсом идёт кандидату, если он знает что такое тротлинг и правильно его применяет. Например так:

import React, { useState, useEffect } from 'react';
import throttle from 'lodash/throttle';

const fetchRandomNumber = () => Promise.resolve(Math.random());

const NumberAndScroll = () => {
  const [number, setNumber] = useState();
 	const [scroll, setScroll] = useState(window.scrollY);
  
  useEffect(() => {
    fetchRandomNumber().then(setNumber);
    
    // обернули в throttle
    const handleScroll = throttle(() => setScroll(window.scrollY), 37);

    window.addEventListener('scroll', handleScroll);
    
    return () => window.removeEventListener('scroll', handleScroll);
  }, []);
  
  return (
  	<div>
      <div> Number: { number } </div>
      <div> Scroll: { scroll } </div>
    </div>	
  )
}

Ещё одна из вещей, которые я бы хотел увидеть - применение Принципа единственной ответственности к useEffect. Сейчас один useEffect отвечает и за логику асинхронного получения числа и за логику скролла. Результат может выглядеть так:

import React, { useState, useEffect } from 'react';
import throttle from 'lodash/throttle';

const fetchRandomNumber = () => Promise.resolve(Math.random());

const NumberAndScroll = () => {
 	const [number, setNumber] = useState();
  const [scroll, setScroll] = useState(window.scrollY);
  
  // отдельно логика получения числа
  useEffect(() => {
    fetchRandomNumber().then(setNumber);
  }, []);

  // отдельно логика скролла
  useEffect(() => {
	  const handleScroll = throttle(() => setScroll(window.scrollY), 37);
    
    window.addEventListener('scroll', handleScroll);
    
    return () => window.removeEventListener('scroll', handleScroll);
  }, []);
  
  return (
  	<div>
      <div> Number: { number } </div>
      <div> Scroll: { scroll } </div>
    </div>	
  )
}

Такой код будет работать незаметно медленней, но его будет значительно проще читать и изменять некоторое время спустя. К тому же после этого шага уже лучше видно кастомный хук который можно вынести для тех же целей:

import React, { useState, useEffect } from 'react';
import throttle from 'lodash/throttle';

const fetchRandomNumber = () => Promise.resolve(Math.random());

// вынесли работу со скроллом в кастомный хук
const useWindowScrollY = () => {
  const [scroll, setScroll] = useState(window.scrollY);

  useEffect(() => {
	  const handleScroll = throttle(() => setScroll(window.scrollY), 37);
    
    window.addEventListener('scroll', handleScroll);
    
    return () => window.removeEventListener('scroll', handleScroll);
  }, []);

	return scroll;
}

const NumberAndScroll = () => {
 	const [number, setNumber] = useState();
  const scroll = useWindowScrollY();
  
  // логику получения числа, возможно, тоже стоило вынести
  // в отдельный кастомный хук
  useEffect(() => {
    fetchRandomNumber().then(setNumber);
  }, []);
  
  return (
  	<div>
      <div> Number: { number } </div>
      <div> Scroll: { scroll } </div>
    </div>	
  )
}

Неочевидное

Есть в JS такая нечасто применяемая в повседневной работе штука как дескрипторы. Можно сделать так, что когда мы достаём из объекта свойство, будет вызвана геттер функция и возвращён результат этого вызова. Функция Object.getOwnPropertyDescriptor может показать нам, является ли свойство объекта обычным или хитро определено дескрипторами. И так, если вы в консоли запустите Object.getOwnPropertyDescriptor(window, 'scrollY');, то увидите, что у свойства scrollY заданы гетер и сетер. То есть когда мы пишем window.scrollY, мы добавляем в стек вызовов эту гетер функцию. То есть если мы напишем useState(() => window.scrollY), то на каждый рендер в стек вызовов будет на одну функцию меньше идти (если передать в useState функцию, то она вызовется только во время первого рендера и начальным значением будет результат этого вызова). Оптимизация! На самом деле такая оптимизация никакой практической пользы не даёт. Даже экономией на спичках не хочется её называть. Но важно тут, как мне кажется, осознание этих процессов, сознательное написание каждой строчки кода. Окончательный вариант кода будет выглядеть примерно так:

import React, { useState, useEffect } from 'react';
import throttle from 'lodash/throttle';

const fetchRandomNumber = () => Promise.resolve(Math.random());

// получение window.scrollY вынесли в функцию
const getWindowScrollY = () => window.scrollY;
const useWindowScrollY = () => {
  const [scroll, setScroll] = useState(getWindowScrollY);

  useEffect(() => {
	  const handleScroll = throttle(() => setScroll(window.scrollY), 37);
    
    window.addEventListener('scroll', handleScroll);
    
    return () => window.removeEventListener('scroll', handleScroll);
  }, []);

	return scroll;
}

const NumberAndScroll = () => {
 	const [number, setNumber] = useState();
  const scroll = useWindowScrollY();
  
  useEffect(() => {
    fetchRandomNumber().then(setNumber);
  }, []);
  
  return (
  	<div>
      <div> Number: { number } </div>
      <div> Scroll: { scroll } </div>
    </div>	
  )
}

Бонус

Тем, кто любит лишний раз пошевелить мозгами, предлагаю придумать как забенчмаркать последнюю "оптимизацию". Я сам никогда не занимался написанием таких синтетических тестов, но специально для полноты этого поста я задался такой целью и вот что вышло у меня: https://codesandbox.io/s/descriptors-perfomance-test-g34e1?file=/src/usePerfomance.js. 20000 рендеров компонента на моём ноутбуке выполняются за ~3500мс (для useState(window.scrollY)) и ~3300мс (для useState(() => window.scrollY))

Заключение

Спасибо всем, кто дочитал