Skip to content

Commit 3b058df

Browse files
ParkreinerParkreiner
committed
refactor: clean up current implementation
1 parent ce8e555 commit 3b058df

File tree

1 file changed

+58
-51
lines changed

1 file changed

+58
-51
lines changed

‎site/src/components/Spinner/Spinner.tsx

Lines changed: 58 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -33,10 +33,24 @@ type SpinnerProps = Readonly<
3333

3434
/**
3535
* Indicates whether the `children` prop should be unmounted during
36-
* a loading state. Defaults to `false` - unmounting HTML elements
37-
* like form controls can lead to invalid HTML, so this prop should
38-
* be used with care and only if it prevents render performance
39-
* issues.
36+
* a loading state. Defaults to `false` - fully unmounting a child
37+
* component has two main risks:
38+
* 1. Hiding children can create invalid HTML. (For example, if you
39+
* have an HTML input and a label, but only hide the input behind
40+
* the spinner during loading state, the label becomes
41+
* "detached"). This not only breaks behavior for screen readers
42+
* but can also create nasty undefined behavior for some built-in
43+
* HTML elements.
44+
* 2. Unmounting a component will cause any of its internal state to
45+
* be completely wiped. Unless the component has all of its state
46+
* controlled by a parent or external state management tool, the
47+
* component will have all its initial state once the loading
48+
* transition ends.
49+
*
50+
* If you do need reset all the state after a loading transition
51+
* and you can't unmount the component without creating invalid
52+
* HTML, use a render key to reset the state.
53+
* @see {@link https://react.dev/learn/you-might-not-need-an-effect#resetting-all-state-when-a-prop-changes}
4054
*/
4155
unmountChildrenWhileLoading?: boolean;
4256

@@ -82,12 +96,49 @@ export const Spinner: FC<SpinnerProps> = ({
8296
unmountChildrenWhileLoading = false,
8397
...delegatedProps
8498
}) => {
99+
// Disallow negative timeout values and fractional values, but also round
100+
// the delay down if it's small enough that it might as well be immediate
101+
// from a user perspective
102+
let safeDelay = Number.isNaN(spinnerDelayMs)
103+
? 0
104+
: Math.min(MAX_SPINNER_DELAY_MS, Math.trunc(spinnerDelayMs));
105+
if (safeDelay < 100) {
106+
safeDelay = 0;
107+
}
108+
/**
109+
* Doing a bunch of mid-render state syncs to minimize risks of UI tearing
110+
* during re-renders. It's ugly, but it's what the React team officially
111+
* recommends.
112+
* @see {@link https://react.dev/learn/you-might-not-need-an-effect#adjusting-some-state-when-a-prop-changes}
113+
*/
114+
const [delayLapsed, setDelayLapsed] = useState(safeDelay === 0);
115+
const canResetLapseOnRerender = delayLapsed && !loading;
116+
if (canResetLapseOnRerender) {
117+
setDelayLapsed(false);
118+
}
119+
// Have to cache delay so that we don't "ping-pong" between state syncs for
120+
// the delayLapsed state and accidentally create an infinite render loop
121+
const [cachedDelay, setCachedDelay] = useState(safeDelay);
122+
const delayWasRemovedOnRerender =
123+
!delayLapsed && safeDelay === 0 && safeDelay !== cachedDelay;
124+
if (delayWasRemovedOnRerender) {
125+
setDelayLapsed(true);
126+
setCachedDelay(safeDelay);
127+
}
128+
useEffect(() => {
129+
if (safeDelay === 0) {
130+
return;
131+
}
132+
const id = window.setTimeout(() => setDelayLapsed(true), safeDelay);
133+
return () => window.clearTimeout(id);
134+
}, [safeDelay]);
135+
const showSpinner = delayLapsed && loading;
136+
85137
// Conditional rendering logic is more convoluted than normal because we
86138
// need to make sure that the children prop is always placed in the same JSX
87139
// "slot" by default, no matter the value of `loading`. Even if the children
88-
// prop value is exactly the same each time, the state will get wiped if the
89-
// placement in the JSX output changes
90-
const showSpinner = useShowSpinner(loading, spinnerDelayMs);
140+
// prop value is exactly the same each time, the state for the associated
141+
// component will get wiped if the parent changes
91142
return (
92143
<>
93144
{showSpinner && (
@@ -137,47 +188,3 @@ export const Spinner: FC<SpinnerProps> = ({
137188
</>
138189
);
139190
};
140-
141-
// Splitting off logic into custom hook so that we can abstract away the chaos
142-
// of handling Spinner's re-render logic. The result is a simple boolean, but
143-
// the steps to calculate that boolean accurately while avoiding re-render
144-
// issues got a little heady
145-
function useShowSpinner(loading: boolean, spinnerDelayMs: number): boolean {
146-
// Disallow negative timeout values and fractional values, but also round
147-
// the delay down if it's small enough that it might as well be immediate
148-
// from a user perspective
149-
let safeDelay = Number.isNaN(spinnerDelayMs)
150-
? 0
151-
: Math.min(MAX_SPINNER_DELAY_MS, Math.trunc(spinnerDelayMs));
152-
if (safeDelay < 100) {
153-
safeDelay = 0;
154-
}
155-
156-
/**
157-
* Doing a bunch of mid-render state syncs to minimize risks of UI tearing
158-
* during re-renders. It's ugly, but it's what the React team officially
159-
* recommends.
160-
* @see {@link https://react.dev/learn/you-might-not-need-an-effect#adjusting-some-state-when-a-prop-changes}
161-
*/
162-
const [delayLapsed, setDelayLapsed] = useState(safeDelay === 0);
163-
const [cachedDelay, setCachedDelay] = useState(safeDelay);
164-
const canResetLapseOnRerender = delayLapsed && !loading;
165-
if (canResetLapseOnRerender) {
166-
setDelayLapsed(false);
167-
}
168-
const delayWasRemovedOnRerender =
169-
!delayLapsed && safeDelay === 0 && safeDelay !== cachedDelay;
170-
if (delayWasRemovedOnRerender) {
171-
setDelayLapsed(true);
172-
setCachedDelay(safeDelay);
173-
}
174-
useEffect(() => {
175-
if (safeDelay === 0) {
176-
return;
177-
}
178-
const id = window.setTimeout(() => setDelayLapsed(true), safeDelay);
179-
return () => window.clearTimeout(id);
180-
}, [safeDelay]);
181-
182-
return delayLapsed && loading;
183-
}

0 commit comments

Comments
 (0)