Wątek przeniesiony 2023-05-18 19:44 z JavaScript przez Riddle.

Uporządkowanie logiki komponentu

0

Witam,
zadaję dość desperackie pytanie, ale myślę, że słuszne. Do sedna. W mojej bibliotece zarządzania stanem mam obiekt nodes. Przechowuje on wszystkie węzły mojego wykresu (React-Flow). Komponent CustomNode jest komponentem renderującym konkretny węzeł w zależności od id.

Problem
Problemem jest to, że (moim zdaniem) ten kod jest dość chaotyczny. Wydaje mi się, że można z nim zrobić coś, co poprawi jego przejrzystość, ale nie wiem do końca jak. Wydaje mi się, że logika jest pomieszana, a w szczególności zarządzania stanem.

Nie wiem, czy dobrze opisałem problem, więc w razie pytań proszę je zadawać.
Nie oczekuję gotowca, a raczej porad jak poprawić kod.

Kod komponentu:

import React, {useEffect, useLayoutEffect, useRef, useState} from 'react';
import {Handle, NodeToolbar} from 'reactflow';
import useBoundStore from '../../store/store';
import Toolbar from './Toolbar';

export default function CustomNode({id}) {
	const inputRef = useRef(null);
	const spanRef = useRef(null);
	const [disabled, setDisabled] = useState(true);
	const [nodes, updateNode] = useBoundStore((state) => [state.nodes, state.updateNode]);
	const [currentNodeData, setCurrentNodeData] = useState({});

	//Update current node data on change
	useEffect(() => {
		if (!nodes) return;
		setCurrentNodeData(nodes.find((el) => el.id === id)?.data);
	}, [nodes?.find((el) => el.id === id)?.data, id]);

	//Auto-width on input value change
	useLayoutEffect(() => {
		inputRef.current.style.width = spanRef.current.offsetWidth + 'px';
	}, [nodes?.find((el) => el.id === id)?.data?.label]);

	//Focus label input on doubleclick
	useEffect(() => {
		if (disabled === true) return;
		inputRef.current.focus();
	}, [disabled]);

	//Unfocus input when click outside
	const onInputBlur = () => {
		const selection = document.getSelection();
		selection.removeAllRanges();
		setDisabled(true);
	};

	return (
		<div className="node" style={{background: currentNodeData?.color}} onDoubleClick={(e) => setDisabled(false)}>
			<Toolbar data={currentNodeData} id={id} />
			<div className="dragHandle">
				<svg viewBox="0 0 24 24">
					<path fill="#333" stroke="#333" strokeWidth="1" d="M15 5h2V3h-2v2zM7 5h2V3H7v2zm8 8h2v-2h-2v2zm-8 0h2v-2H7v2zm8 8h2v-2h-2v2zm-8 0h2v-2H7v2z" />
				</svg>
			</div>

			<input
				type="text"
				value={currentNodeData?.label || ''}
				onChange={(e) => updateNode({id, propety: 'label', value: e.target.value})}
				onBlur={(e) => onInputBlur(e)}
				disabled={disabled}
				ref={inputRef}
			/>

			<Handle type="source" position="top" className="source" />

			<span ref={spanRef} className="autoWidth">
				{nodes?.find((el) => el.id === id)?.data?.label}
			</span>
		</div>
	);
}

6
  1. useEffect() który nie robi nic poza ustawieniem stanu najczęściej możesz zastąpić useMemo(), korzyści:
  • zmiana, której oczekujesz nastąpi jeden render wcześniej
  • w ogóle będzie jeden render mniej
  • masz jeden useState mniej w kodzie
  1. setDisable() niby powoduje, że kod łatwiej się czyta "jak książkę" (choć kolejność jest zupełnie nieksiążkowa), niemniej w praktyce nie jest ten stan Ci za bardzo potrzebny, jedyne do czego służy to focusowanie elementu. Sfocusuj go od razu w onDoubleClick={...}, pozbędziesz się:
  • kolejnego useEffect()
  • kolejnego useState()
  • kolejny render mniej
  • w efekcie focus nastąpi "jedną klatkę" (to trochę umowne) szybciej
  1. Wyszukiwanie w tablicach jest szybkie w JS, ale mimo wszystko jak widzę kilkukrotne nodes?.find((el) => el.id === id) to trochę boli. Aczkolwiek wyciągnięcie tego do współdzielonego consta niekoniecznie poprawi czytelność, raczej przeciwnie. Zauważ, że używasz tego również w JSX na dole, więc iterujesz po tablicy (choć nie całej, 3x per render!)

  2. Wywaliłbym ikonę do osobnego komponentu + pliku

  3. onBlur={(e) => onInputBlur(e)} możesz zastąpić onBlue={onInputBlur}

Szału to nie zrobi, ale jednak jak zastosujesz to wszystko to będzie czytelniej i wydajniej.

0

Bardzo dziękuję za odpowiedź. O to mi chodziło.

0

Tak sobie przeglądam kod, który napisałem po Twojej odpowiedzi i natchnęło mnie na przemyślenia. Czy odnośnie punktu numer 1 powinienem robić tak:

const currentNodeData = useMemo(() => nodes?.find((el) => el.id === id)?.data, [nodes?.find((el) => el.id === id)?.data]);

, czy tak:

const currentNodeData = useMemo(() => nodes?.find((el) => el.id === id)?.data, [nodes]);
1

Tak szczerze to dopiero teraz przyjrzałem się temu dawnemu useEffect i teraz useMemo i ... cały ten useMemo też nie ma sensu, nie oszczędzasz tutaj kompletnie nic, a w zasadzie możesz pogorszyć wydajność, bo masz dwie iteracje po tablicy.

Drugi zapis może mieć sens - wszystko zależy co i kiedy powoduje zmianę nodes. Jeżeli w wersji drugiej jest wszystko ok to użyj drugiej.

Niemniej pierwszy zapis trochę sugeruje, że pominąłeś punkt 3. z mojej listy wyżej. Proponuję tak - wrzuć obecną formę tego komponentu, zobaczymy co dalej można ulepszyć.

0

Jeśli chodzi o zmianę stanu nodes to są 2 czynniki:

  1. Zmiana wywołana przez bibliotekę React-Flow - na przykład przeciągnięcie węzła, które aktualizuje właściwość position
  2. Zmiana wywołana przez moją logikę - na przykład zmiana etykiety węzła, koloru itp. Właściwości, które są zmieniane przeze mnie są przechowywane w obiekcie data, więc są scentralizowane. Dla przypomnienia tablica nodes przechowuje obiekty węzłów mojego wykresu.
    Przykładowy obiekt nodes:
{
    "id": "root",
    "type": "custom",
    "data": {
        "label": "Node 1",
        "backgroundColor": "#ffffff",
        "color": "#000000"
    },
    "position": {
        "x": 0,
        "y": 0
    },
    "dragHandle": ".dragHandle",
    "width": 123,
    "height": 29
}

Obecna wersja komponentu:

import React, {memo, useLayoutEffect, useMemo, useRef} from 'react';
import {Handle} from 'reactflow';
import useBoundStore from '../../store/store';
import Toolbar from './Toolbar';

function CustomNode({id}) {
	const inputRef = useRef(null);
	const spanRef = useRef(null);
	const [nodes, updateNode] = useBoundStore((state) => [state.nodes, state.updateNode]);
	const currentNodeData = useMemo(() => nodes?.find((el) => el.id === id)?.data, [nodes?.find((el) => el.id === id)?.data]);

	//Auto-width on input value change
	useLayoutEffect(() => {
		inputRef.current.style.width = spanRef.current.offsetWidth + 'px';
	}, [currentNodeData]);

	return (
		<div className="node" style={{backgroundColor: currentNodeData?.backgroundColor}} onDoubleClick={() => inputRef.current.focus()}>
			<Toolbar data={currentNodeData} id={id} />

			<span className="dragHandle material-symbols-rounded" style={{color: currentNodeData?.color}}>
				drag_indicator
			</span>

			<input
				type="text"
				ref={inputRef}
				value={currentNodeData?.label || ''}
				onChange={(e) => updateNode({id, propety: 'label', value: e.target.value})}
				style={{color: currentNodeData?.color, fontWeight: currentNodeData?.bold ? 'bold' : 'normal', fontStyle: currentNodeData?.italic ? 'italic' : 'normal'}}
			/>

			<span ref={spanRef} className="autoWidth">
				{currentNodeData?.label}
			</span>
			<Handle type="source" position="top" className="source" />
		</div>
	);
}

export default memo(CustomNode);

0

Ok, wszystko brzmi całkiem ok, i "zgodnie ze sztuką", więc najoptymalniej będzie tam dać:

const currentNodeData = useMemo(() => nodes?.find((el) => el.id === id)?.data, [nodes]);

i tyle, nie widzę tam nic więcej do poprawy

0

No dobrze, ale czy to będzie wydajne? position jest aktualizowane co każde przesunięcie węzła, wiec nawet kilkanaście razy na sekundę. Wtedy i currentNodeData będzie aktualizowane mimo, że position mnie nie interesuje.

Może korzystając z tego, że każdy węzeł ma swój identyfikator, przeniosę potrzebne mi dane węzłów do oddzielnej tablicy i będę wyszukiwał po id? Co o tym myślisz?

Przykład takiego podejścia, który działa u mnie
Mój sklep zustand:

nodesData: [
		{
			id: 'root',
			data: {label: 'Node 1', backgroundColor: '#ffffff', color: '#000000'},
		},
		{
			id: '1',
			data: {label: 'Website', backgroundColor: '#ffffff', color: '#000000'},
		},
	],
	nodes: [
		{
			id: 'root',
			type: 'custom',
			position: {x: 0, y: 0},
			dragHandle: '.dragHandle',
		},
		{
			id: '1',
			type: 'custom',
			position: {x: -20, y: -110},
			dragHandle: '.dragHandle',
		},
	],

, wykorzystanie tego w komponencie:

const currentNodeData = useMemo(() => nodesData?.find((el) => el.id === id)?.data, [nodesData]);

oraz funkcja aktualizująca dane:

updateNode: ({id, propety, value}) => {
		set((state) => {
			const newNodesData = state.nodesData.map((el) => (el.id === id ? {...el, data: {...el.data, [propety]: value}} : el));
			return {...state, nodesData: newNodesData};
		});
	},

Co myślisz o takim podejściu? Nie mam doświadczenia w optymalizowaniu komponentów, ale po głośności mojego wentylatora słyszę różnicę.

0

No dobrze, ale czy to będzie wydajne? position jest aktualizowane co każde przesunięcie węzła, wiec nawet kilkanaście razy na sekundę. Wtedy i currentNodeData będzie aktualizowane mimo, że position mnie nie interesuje.

W momencie jak nodes się zmienia to tak czy siak komponent zostanie przerenderowany.

Ponieważ nie używasz currentNodeData.position nigdzie, to nie wywoła to żadnej zmiany w DOM, ale tak czy siak - cała treść komponentu CustomNode zostanie odpalona.

po głośności mojego wentylatora słyszę różnicę

Nawet jak masz dobry słuch, to to jest zgadywanie. Nie zgadujemy, mierzymy! Zakładka performance w dev toolsach, na youtube czy gdziekolwiek znajdziesz tutoriale jak z tego mniej więcej korzystać.
Zmiana w menedżerze stanu wydaje się być ok, jeżeli faktycznie oszczędza ilość renderów to super. Jedyny problem to taka trochę upierdliwość w synchronizowaniu elementów w nodes i nodesData.

Ale wracając do nasłuchiwania wiatraków - trzeba szukać balansu między wydajnością, a czytelnością i możliwością utrzymania kodu. Jeżeli wchodzimy na ekstrema optymalizacji najczęściej obniżamy czytelność kodu i utrudniamy przyszłe zmiany, często niewinne zmiany mogą zepsuć całą optymalizację. Jeżeli zmierzysz (nie na ucho, w zakładce Performance), że różnica jest żadna lub niewielka - być może warto zostawić to tak jak jest.

1 użytkowników online, w tym zalogowanych: 0, gości: 1