Misc updates to better handle tearing and add some future TODOs
Brian Vaughn committed
Jan 27, 2019 at 07:58 UTC
b7cbd382147d4941381535d1dc18db9abe98b6ba
11 files changed
+110
-32
shells/browser/shared/src/main.js
+3
-1
@@ -15,7 +15,9 @@ function createPanelIfReactLoaded() {
15
16
clearInterval(loadCheckInterval);
17
panelCreated = true;
18
- chrome.devtools.panels.create('React', '', 'panel.html', function(panel) {
18
+ chrome.devtools.panels.create('⚛ Elements', '', 'panel.html', function(
19
+ panel
20
+ ) {
21
panel.onShown.addListener(function(window) {
22
// TODO: When the user switches to the panel, check for an Elements tab selection.
23
});
shells/browser/shared/src/panel.js
+2
-2
@@ -3,7 +3,7 @@
3
import { createElement } from 'react';
4
import { render } from 'react-dom';
5
import Bridge from 'src/bridge';
6
-import App from 'src/devtools/views/App';
6
+import Elements from 'src/devtools/views/Elements';
7
import inject from './inject';
8
9
const IS_CHROME = navigator.userAgent.indexOf('Firefox') < 0;
@@ -58,5 +58,5 @@ inject(chrome.runtime.getURL('build/backend.js'), () => {
58
},
59
});
60
61
- render(createElement(App, { bridge, browserName, themeName }), node);
61
+ render(createElement(Elements, { bridge, browserName, themeName }), node);
62
});
shells/dev/app/index.js
+14
-3
@@ -1,11 +1,22 @@
1
/** @flow */
2
3
import { createElement } from 'react';
4
-import { render } from 'react-dom';
4
+import { render, unmountComponentAtNode } from 'react-dom';
5
import App from './App';
6
7
const container = document.createElement('div');
8
9
-render(createElement(App), container);
10
-
9
((document.body: any): HTMLBodyElement).appendChild(container);
10
+
11
+function mountTestApp() {
12
+ render(createElement(App), container);
13
+}
14
+
15
+function unmountTestApp() {
16
+ unmountComponentAtNode(container);
17
+}
18
+
19
+mountTestApp();
20
+
21
+window.parent.mountTestApp = mountTestApp;
22
+window.parent.unmountTestApp = unmountTestApp;
shells/dev/index.html
+2
@@ -30,6 +30,8 @@
30
</style>
31
</head>
32
<body>
33
+ <button id="mountButton">Unmount test app</button>
34
+
35
<!-- React test app (shells/dev/app) is injected here -->
36
<!-- DevTools backend (shells/dev/src) is injected here -->
37
<!-- global "hook" is defined on the iframe's contentWindow -->
shells/dev/src/devtools.js
+30
-6
@@ -1,11 +1,12 @@
1
/** @flow */
2
3
import { createElement } from 'react';
4
-import { render } from 'react-dom';
4
+// $FlowFixMe Flow does not yet know about createRoot()
5
+import { createRoot } from 'react-dom';
6
import Bridge from 'src/bridge';
7
import { installHook } from 'src/hook';
8
import { initDevTools } from 'src/devtools';
8
-import App from 'src/devtools/views/App';
9
+import Elements from 'src/devtools/views/Elements';
10
11
const iframe = ((document.getElementById('target'): any): HTMLIFrameElement);
12
@@ -13,6 +14,29 @@ const { contentDocument, contentWindow } = iframe;
14
15
installHook(contentWindow);
16
17
+const container = ((document.getElementById('devtools'): any): HTMLElement);
18
+
19
+let isTestAppMounted = true;
20
+
21
+const mountButton = ((document.getElementById(
22
+ 'mountButton'
23
+): any): HTMLButtonElement);
24
+mountButton.addEventListener('click', function() {
25
+ if (isTestAppMounted) {
26
+ if (typeof window.unmountTestApp === 'function') {
27
+ window.unmountTestApp();
28
+ mountButton.innerText = 'Mount test app';
29
+ isTestAppMounted = false;
30
+ }
31
+ } else {
32
+ if (typeof window.mountTestApp === 'function') {
33
+ window.mountTestApp();
34
+ mountButton.innerText = 'Unmount test app';
35
+ isTestAppMounted = true;
36
+ }
37
+ }
38
+});
39
+
40
initDevTools({
41
connect(cb) {
42
inject('./build/backend.js', () => {
@@ -29,13 +53,13 @@ initDevTools({
53
54
cb(bridge);
55
32
- render(
33
- createElement(App, {
56
+ const root = createRoot(container);
57
+ root.render(
58
+ createElement(Elements, {
59
bridge,
60
browserName: 'Chrome',
61
themeName: 'light',
37
- }),
38
- ((document.getElementById('devtools'): any): HTMLElement)
62
+ })
63
);
64
});
65
},
src/devtools/store.js
+10
-5
@@ -17,7 +17,9 @@ class Store extends EventEmitter {
17
_idToElement: Map<string, Element> = new Map();
18
_idToParentID: Map<string, string> = new Map();
19
20
- roots: Set<string> = new Set();
20
+ // This Array must be treated as immutable!
21
+ // Passive effects will check it for changes between render and mount.
22
+ roots: $ReadOnlyArray<string> = [];
23
24
constructor(bridge: Bridge) {
25
super();
@@ -50,8 +52,8 @@ class Store extends EventEmitter {
52
53
onBridgeRoot = (id: string) => {
54
debug('onBridgeRoot()', id);
53
- if (!this.roots.has(id)) {
54
- this.roots.add(id);
55
+ if (!this.roots.includes(id)) {
56
+ this.roots = this.roots.concat(id);
57
this.emit('roots');
58
}
59
};
@@ -64,8 +66,11 @@ class Store extends EventEmitter {
66
this._idToParentID.delete(id);
67
}
68
67
- if (this.roots.has(id)) {
68
- this.roots.delete(id);
69
+ const index = this.roots.indexOf(id);
70
+ if (index >= 0) {
71
+ this.roots = this.roots
72
+ .slice(0, index)
73
+ .concat(this.roots.slice(index + 1));
74
this.emit('roots');
75
}
76
};
src/devtools/views/Element.js
+8
@@ -15,6 +15,14 @@ export default function Element({ depth, id }: Props) {
15
const store = useContext(StoreContext);
16
const element = useElement(store, id);
17
18
+ // DevTools are rendered in concurrent mode.
19
+ // It's possible the store has updated since the commit that triggered this render.
20
+ // So we need to guard against an undefined element.
21
+ // TODO: Handle this by switching to a Suspense based approach.
22
+ if (element == null) {
23
+ return null;
24
+ }
25
+
26
const { children, displayName, key } = element;
27
28
// TODO: Add state for toggling element open/close
src/devtools/views/Elements.css
renamed
+1
-1
@@ -37,7 +37,7 @@
37
--font-size-tree-normal: 14px;
38
}
39
40
-.App {
40
+.Elements {
41
width: 100%;
42
height: 100%;
43
overflow: auto;
src/devtools/views/Elements.js
renamed
+3
-3
@@ -4,7 +4,7 @@ import React, { useMemo } from 'react';
4
import Store from '../store';
5
import Tree from './Tree';
6
import { StoreContext } from './contexts';
7
-import styles from './App.css';
7
+import styles from './Elements.css';
8
9
import type { Bridge } from '../../types';
10
@@ -14,12 +14,12 @@ export type Props = {|
14
themeName: string,
15
|};
16
17
-export default function App({ bridge, browserName, themeName }: Props) {
17
+export default function Elements({ bridge, browserName, themeName }: Props) {
18
const store = useMemo(() => new Store(bridge), []);
19
20
return (
21
<StoreContext.Provider value={store}>
22
- <div className={styles.App}>
22
+ <div className={styles.Elements}>
23
<Tree />
24
</div>
25
</StoreContext.Provider>
src/devtools/views/Tree.js
+8
@@ -30,6 +30,14 @@ function Root({ id }: RootProps) {
30
const store = useContext(StoreContext);
31
const element = useElement(store, id);
32
33
+ // DevTools are rendered in concurrent mode.
34
+ // It's possible the store has updated since the commit that triggered this render.
35
+ // So we need to guard against an undefined element.
36
+ // TODO: Handle this by switching to a Suspense based approach.
37
+ if (element == null) {
38
+ return null;
39
+ }
40
+
41
return element.children.map(childID => (
42
<Element key={childID} depth={0} id={childID} />
43
));
src/devtools/views/hooks.js
+29
-11
@@ -5,30 +5,48 @@ import { useLayoutEffect, useState } from 'react';
5
import type { Element } from '../types';
6
import Store from '../store';
7
8
-export function useElement(store: Store, id: string): Element {
9
- const [element, setElement] = useState<Element>(
10
- ((store.getElement(id): any): Element)
11
- );
8
+// TODO useEffect has a bug where sometimes subscriptions don't get cleaned up correctly.
9
+// Potentially related to github.com/facebookincubator/redux-react-hook/issues/17
10
+// As a temporary work around, switch back to layout effect.
11
+
12
+export function useElement(store: Store, id: string): ?Element {
13
+ const [element, setElement] = useState<?Element>(store.getElement(id));
14
13
- // TODO: We might still miss updates in concurrent mode.
14
- // We should just useEffect and do a sync comparison (like in create-subscription) to handle this.
15
useLayoutEffect(() => {
16
const handler = () => setElement(((store.getElement(id): any): Element));
17
+
18
+ // Listen for changes to the element.
19
store.addListener(id, handler);
20
+
21
+ // Check for changes that may have happened between render and mount.
22
+ const newElement = store.getElement(id);
23
+ if (element !== newElement) {
24
+ setElement(newElement);
25
+ }
26
+
27
+ // Remove event listener on unmount.
28
return () => store.removeListener(id, handler);
29
}, [store, id]);
30
31
return element;
32
}
33
24
-export function useRoots(store: Store): Array<string> {
25
- const [roots, setRoots] = useState<Array<string>>(Array.from(store.roots));
34
+export function useRoots(store: Store): $ReadOnlyArray<string> {
35
+ const [roots, setRoots] = useState<$ReadOnlyArray<string>>(store.roots);
36
27
- // TODO: We might still miss updates in concurrent mode.
28
- // We should just useEffect and do a sync comparison (like in create-subscription) to handle this.
37
useLayoutEffect(() => {
30
- const handler = () => setRoots(Array.from(store.roots));
38
+ const handler = () => setRoots(store.roots);
39
+
40
+ // Listen for changes to roots.
41
store.addListener('roots', handler);
42
+
43
+ // Check for changes that may have happened between render and mount.
44
+ const newRoots = store.roots;
45
+ if (roots !== newRoots) {
46
+ setRoots(newRoots);
47
+ }
48
+
49
+ // Remove event listener on unmount.
50
return () => store.removeListener('roots', handler);
51
}, [store]);
52