Warn if MutableSource snapshot is a function (#18933)
* Warn if MutableSource snapshot is a function useMutableSource does not properly support snapshots that are functions. In part this is because of how it is implemented internally (the function gets mistaken for a state updater function). To fix this we could just wrap another function around the returned snapshot, but this pattern seems problematic to begin with- because the function that gets returned might itself close over mutable values, which would defeat the purpose of using the hook in the first place. This PR proposes adding a new DEV warning if the snapshot returned is a function. It does not change the behavior (meaning that a function could still work in some cases- but at least the current behavior prevents passing around a closure that may later become stale unless you're really intentional about it e.g. () => () => {...}). * Replaced .warn with .error
Brian Vaughn committed
May 21, 2020 at 16:14 UTC
8f4dc3e5d005459058ed7ffc26c2fb76b845ce62
3 files changed
+66
-2
packages/react-reconciler/src/ReactFiberHooks.new.js
+19
-1
@@ -914,7 +914,16 @@ function readFromUnsubcribedMutableSource<Source, Snapshot>(
914
}
915
916
if (isSafeToReadFromSource) {
917
- return getSnapshot(source._source);
917
+ const snapshot = getSnapshot(source._source);
918
+ if (__DEV__) {
919
+ if (typeof snapshot === 'function') {
920
+ console.error(
921
+ 'Mutable source should not return a function as the snapshot value. ' +
922
+ 'Functions may close over mutable values and cause tearing.',
923
+ );
924
+ }
925
+ }
926
+ return snapshot;
927
} else {
928
// This handles the special case of a mutable source being shared beween renderers.
929
// In that case, if the source is mutated between the first and second renderer,
@@ -992,6 +1001,15 @@ function useMutableSource<Source, Snapshot>(
1001
const maybeNewVersion = getVersion(source._source);
1002
if (!is(version, maybeNewVersion)) {
1003
const maybeNewSnapshot = getSnapshot(source._source);
1004
+ if (__DEV__) {
1005
+ if (typeof maybeNewSnapshot === 'function') {
1006
+ console.error(
1007
+ 'Mutable source should not return a function as the snapshot value. ' +
1008
+ 'Functions may close over mutable values and cause tearing.',
1009
+ );
1010
+ }
1011
+ }
1012
+
1013
if (!is(snapshot, maybeNewSnapshot)) {
1014
setSnapshot(maybeNewSnapshot);
1015
packages/react-reconciler/src/ReactFiberHooks.old.js
+19
-1
@@ -900,7 +900,16 @@ function readFromUnsubcribedMutableSource<Source, Snapshot>(
900
}
901
902
if (isSafeToReadFromSource) {
903
- return getSnapshot(source._source);
903
+ const snapshot = getSnapshot(source._source);
904
+ if (__DEV__) {
905
+ if (typeof snapshot === 'function') {
906
+ console.error(
907
+ 'Mutable source should not return a function as the snapshot value. ' +
908
+ 'Functions may close over mutable values and cause tearing.',
909
+ );
910
+ }
911
+ }
912
+ return snapshot;
913
} else {
914
// This handles the special case of a mutable source being shared beween renderers.
915
// In that case, if the source is mutated between the first and second renderer,
@@ -978,6 +987,15 @@ function useMutableSource<Source, Snapshot>(
987
const maybeNewVersion = getVersion(source._source);
988
if (!is(version, maybeNewVersion)) {
989
const maybeNewSnapshot = getSnapshot(source._source);
990
+ if (__DEV__) {
991
+ if (typeof maybeNewSnapshot === 'function') {
992
+ console.error(
993
+ 'Mutable source should not return a function as the snapshot value. ' +
994
+ 'Functions may close over mutable values and cause tearing.',
995
+ );
996
+ }
997
+ }
998
+
999
if (!is(snapshot, maybeNewSnapshot)) {
1000
setSnapshot(maybeNewSnapshot);
1001
packages/react-reconciler/src/__tests__/useMutableSource-test.internal.js
+28
@@ -1493,6 +1493,34 @@ describe('useMutableSource', () => {
1493
},
1494
);
1495
1496
+ // @gate experimental
1497
+ it('warns about functions being used as snapshot values', async () => {
1498
+ const source = createSource(() => 'a');
1499
+ const mutableSource = createMutableSource(source);
1500
+
1501
+ const getSnapshot = () => source.value;
1502
+
1503
+ function Read() {
1504
+ const fn = useMutableSource(mutableSource, getSnapshot, defaultSubscribe);
1505
+ const value = fn();
1506
+ Scheduler.unstable_yieldValue(value);
1507
+ return value;
1508
+ }
1509
+
1510
+ const root = ReactNoop.createRoot();
1511
+ await act(async () => {
1512
+ root.render(
1513
+ <>
1514
+ <Read />
1515
+ </>,
1516
+ );
1517
+ expect(() => expect(Scheduler).toFlushAndYield(['a'])).toErrorDev(
1518
+ 'Mutable source should not return a function as the snapshot value.',
1519
+ );
1520
+ });
1521
+ expect(root).toMatchRenderedOutput('a');
1522
+ });
1523
+
1524
// @gate experimental
1525
it('getSnapshot changes and then source is mutated during interleaved event', async () => {
1526
const {useEffect} = React;