Skip to content

Commit 9fbfa7b

Browse files
compulimcompulim
committed
Fix setState in render loop
1 parent 73332a3 commit 9fbfa7b

6 files changed

Lines changed: 109 additions & 24 deletions

File tree

‎__tests__/html2/simple/fatModule/otherCDN.skip.html‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
<!doctype html>
2+
<html lang="en-US">
3+
<head>
4+
<link href="/assets/index.css" rel="stylesheet" type="text/css" />
5+
</head>
6+
<body>
7+
<main id="webchat"></main>
8+
<script type="importmap">
9+
{
10+
"imports": {
11+
"botframework-webchat": "/__dist__/packages/bundle/static/botframework-webchat.js",
12+
"react": "/__dist__/packages/bundle/static/react.js",
13+
"react-dom": "/__dist__/packages/bundle/static/react-dom.js"
14+
}
15+
}
16+
</script>
17+
<script type="importmap-unpkg">
18+
{
19+
"imports": {
20+
"botframework-webchat": "https://unpkg.com/botframework-webchat@main/static/botframework-webchat.js",
21+
"react": "https://unpkg.com/botframework-webchat@main/static/react.js",
22+
"react-dom": "https://unpkg.com/botframework-webchat@main/static/react-dom.js"
23+
}
24+
}
25+
</script>
26+
<script type="importmap-jsdelivr">
27+
{
28+
"imports": {
29+
"botframework-webchat": "https://cdn.jsdelivr.net/npm/botframework-webchat@main/static/botframework-webchat.js",
30+
"react": "https://cdn.jsdelivr.net/npm/botframework-webchat@main/static/react.js",
31+
"react-dom": "https://cdn.jsdelivr.net/npm/botframework-webchat@main/static/react-dom.js"
32+
}
33+
}
34+
</script>
35+
<script type="importmap-esmsh">
36+
{
37+
"imports": {
38+
"botframework-webchat": "https://esm.sh/botframework-webchat@main/static/botframework-webchat.js?deps=react@18.3.1,react-dom@18.3.1&bundle=false",
39+
"react": "https://esm.sh/botframework-webchat@main/static/react.js",
40+
"react-dom": "https://esm.sh/botframework-webchat@main/static/react-dom.js"
41+
}
42+
}
43+
</script>
44+
<script type="module">
45+
import '/test-harness.mjs';
46+
import '/test-page-object.mjs';
47+
48+
import { createDirectLine, createStoreWithOptions, renderWebChat } from 'botframework-webchat';
49+
import { version } from 'react';
50+
51+
run(async function () {
52+
const {
53+
testHelpers: { createDirectLineEmulator }
54+
} = window;
55+
56+
// TODO: This is for `createDirectLineEmulator` only, should find ways to eliminate this line.
57+
window.WebChat = { createStoreWithOptions };
58+
59+
const { directLine, store } = createDirectLineEmulator();
60+
61+
renderWebChat({ directLine, store }, document.getElementById('webchat'));
62+
63+
await pageConditions.uiConnected();
64+
65+
await directLine.emulateIncomingActivity('Hello, World!');
66+
67+
await pageConditions.numActivitiesShown(1);
68+
69+
await host.snapshot('local');
70+
71+
expect(typeof globalThis.React).toBe('undefined');
72+
expect(version).toBe('18.3.1');
73+
});
74+
</script>
75+
</body>
76+
</html>

‎packages/bundle/esbuild.static.mjs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ function createWatcherPlugin(name) {
5454
/** @type { Map<string, import('esbuild').BuildOptions> } */
5555
const configs = new Map();
5656

57+
/** @type { import('esbuild').BuildOptions } */
5758
const BASE_CONFIG = {
5859
alias: {
5960
adaptivecards: '@msinternal/adaptivecards',
@@ -69,6 +70,7 @@ const BASE_CONFIG = {
6970
entryNames: '[dir]/[name]',
7071
external: ['react', 'react-dom'],
7172
format: 'esm',
73+
keepNames: true,
7274
loader: { '.js': 'jsx' },
7375
minify: true,
7476
outdir: resolve(fileURLToPath(import.meta.url), `../static/`),

‎packages/component/src/BasicTranscript.tsx‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import React, {
88
Fragment,
99
memo,
1010
useCallback,
11+
useEffect,
1112
useMemo,
1213
useRef,
1314
type KeyboardEventHandler,
@@ -483,15 +484,14 @@ const InternalTranscriptScrollable = ({ children, onFocusFiller }: InternalTrans
483484

484485
const stickyChangedToTrue = prevSticky !== sticky && sticky;
485486

486-
useMemo(
487-
() =>
488-
stickyChangedToTrue &&
487+
// This should not loop because usePrevious() will set "stickyChangedToTrue" to "false" on next render.
488+
useEffect(() => {
489+
stickyChangedToTrue &&
489490
// TODO: [P2] Both `markActivityKeyAsRead` and `markAllAsAcknowledged` hook are setters of useState.
490491
// This means, in a render loop, we will be calling setter and will cause another re-render.
491492
// This is not trivial but we should think if there is a way to avoid this.
492-
markAllAsAcknowledged(),
493-
[markAllAsAcknowledged, stickyChangedToTrue]
494-
);
493+
markAllAsAcknowledged();
494+
}, [markAllAsAcknowledged, stickyChangedToTrue]);
495495

496496
// We need to check if `children` is `false` or not.
497497
// If `children` is `false`, React.Children.count(children) will still return 1 (truthy).

‎packages/component/src/ChatHistory/private/ScrollToEndButton.tsx‎

Lines changed: 19 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { hooks } from 'botframework-webchat-api';
2-
import React, { Fragment, memo, MutableRefObject, useCallback, useMemo } from 'react';
2+
import React, { Fragment, memo, MutableRefObject, useCallback, useEffect, useMemo } from 'react';
33
import { useAnimatingToEnd, useAtEnd, useScrollToEnd, useSticky } from 'react-scroll-to-bottom';
44
import { useRefFrom } from 'use-ref-from';
55

@@ -8,6 +8,8 @@ import useFocusByActivityKey from '../../providers/TranscriptFocus/useFocusByAct
88

99
const { useActivityKeysByRead, useCreateScrollToEndButtonRenderer, useMarkActivityKeyAsRead, useStyleOptions } = hooks;
1010

11+
const EMPTY_ARRAY = Object.freeze([]);
12+
1113
const useScrollToEndRenderResult = (terminatorRef: MutableRefObject<HTMLDivElement>) => {
1214
const [renderingActivityKeys] = useRenderingActivityKeys();
1315
const [sticky]: [boolean] = useSticky();
@@ -46,18 +48,22 @@ const useScrollToEndRenderResult = (terminatorRef: MutableRefObject<HTMLDivEleme
4648

4749
// To prevent flashy button, we are not waiting for another render loop to update the `[readActivityKeys, unreadActivityKeys]` state.
4850
// Instead, we are building the next one in this `useMemo` call.
49-
const nextUnreadActivityKeys = useMemo(() => {
50-
// This code need to be careful reviewed as it will cause another render. The code should be converging.
51-
// After we call `markActivityKeyAsRead`, everything will be read and nothing will be unread.
52-
// That means, in next render, `unreadActivityKeys` will be emptied and the `markActivityKeyAsRead` will not get called again.
53-
if (sticky && unreadActivityKeys.length) {
54-
markActivityKeyAsRead(unreadActivityKeys[unreadActivityKeys.length - 1]);
55-
56-
return [];
57-
}
51+
const [nextUnreadActivityKeys, activityKeyToMarkAsUnread] = useMemo<readonly [readonly string[], string | undefined]>(
52+
() =>
53+
Object.freeze(
54+
sticky && unreadActivityKeys.length
55+
? [EMPTY_ARRAY, unreadActivityKeys[unreadActivityKeys.length - 1]]
56+
: [unreadActivityKeys, undefined]
57+
),
58+
[sticky, unreadActivityKeys]
59+
);
5860

59-
return unreadActivityKeys;
60-
}, [markActivityKeyAsRead, sticky, unreadActivityKeys]);
61+
// This code need to be careful reviewed as it will cause another render. The code should be converging.
62+
// After we call `markActivityKeyAsRead`, everything will be read and nothing will be unread.
63+
// That means, in next render, `unreadActivityKeys` will be emptied and the `markActivityKeyAsRead` will not get called again.
64+
useEffect(() => {
65+
activityKeyToMarkAsUnread && markActivityKeyAsRead(activityKeyToMarkAsUnread);
66+
}, [activityKeyToMarkAsUnread, markActivityKeyAsRead]);
6167

6268
const nextUnreadActivityKeysRef = useRefFrom(nextUnreadActivityKeys);
6369

@@ -99,6 +105,7 @@ const useScrollToEndRenderResult = (terminatorRef: MutableRefObject<HTMLDivEleme
99105

100106
function ScrollToEndButton({ terminatorRef }: Readonly<{ terminatorRef: MutableRefObject<HTMLDivElement> }>) {
101107
const children = useScrollToEndRenderResult(terminatorRef);
108+
102109
return <Fragment>{children}</Fragment>;
103110
}
104111

‎packages/component/src/providers/LiveRegionTwin/useLiveRegion.ts‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { useMemo, type DependencyList, type ReactElement } from 'react';
1+
import { useEffect, type DependencyList, type ReactElement } from 'react';
22
import { useRefFrom } from 'use-ref-from';
33

44
import useQueueStaticElement from './useQueueStaticElement';
@@ -18,9 +18,12 @@ export default function useLiveRegion(
1818
const createNodeRef = useRefFrom(createNode);
1919
const queueStaticElement = useQueueStaticElement();
2020

21-
useMemo(() => {
21+
// Unless "deps" change on every render, this should not cause dead loop.
22+
// TODO: [P1] Can we build a `useEffectWithDeadLoopDetection()`?
23+
useEffect(() => {
2224
const node = createNodeRef?.current();
2325

26+
// "queueStaticElement()" will "setState", check for dead loop.
2427
node && queueStaticElement(node);
2528
// eslint-disable-next-line react-hooks/exhaustive-deps
2629
}, [...deps, createNodeRef, queueStaticElement]);

‎packages/test/harness/src/host/common/host/done.js‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -25,10 +25,7 @@ module.exports = (webDriver, resolve) =>
2525
!(
2626
message.includes(
2727
'Support for defaultProps will be removed from function components in a future major release. Use JavaScript default parameters instead.'
28-
) ||
29-
message.includes('ReactDOM.render is no longer supported in React 18. Use createRoot instead.') ||
30-
// TODO: [P0] We should fix the "Cannot update a component while rendering a different component" error.
31-
(message.includes('Cannot update a component') && message.includes('while rendering a different component'))
28+
) || message.includes('ReactDOM.render is no longer supported in React 18. Use createRoot instead.')
3229
)
3330
);
3431

0 commit comments

Comments
 (0)