Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

useMutableSource: bugfix for new getSnapshot with mutation #18297

Merged
merged 5 commits into from Apr 3, 2020

Conversation

@bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Mar 13, 2020

Pulls in @acdlite's failing tests from #18296.

Fixes one, leaves TODO comment for addressing the other two once we have a mechanism to do so. (Currently some temporary tearing is possible between passive effect flushes.)

@bvaughn bvaughn requested a review from acdlite Mar 13, 2020
@bvaughn bvaughn force-pushed the bvaughn:useMutableSource-bugfix branch from e07253b to bc57bac Mar 13, 2020
@codesandbox
Copy link

@codesandbox codesandbox bot commented Mar 13, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit e07253b:

Sandbox Source
sad-keller-9ht8j Configuration
@codesandbox
Copy link

@codesandbox codesandbox bot commented Mar 13, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 86da5ce:

Sandbox Source
compassionate-thompson-v9pst Configuration
@sizebot
Copy link

@sizebot sizebot commented Mar 13, 2020

Details of bundled changes.

Comparing: a8f2165...86da5ce

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer.development.js +0.4% +0.4% 560.52 KB 562.52 KB 116.6 KB 117.07 KB UMD_DEV
react-test-renderer.production.min.js 🔺+0.6% 🔺+0.5% 73.33 KB 73.76 KB 22.32 KB 22.43 KB UMD_PROD
react-test-renderer-shallow.development.js 0.0% 0.0% 38.63 KB 38.63 KB 9.4 KB 9.4 KB UMD_DEV
react-test-renderer-shallow.production.min.js 0.0% 0.0% 12.74 KB 12.74 KB 3.97 KB 3.97 KB UMD_PROD
react-test-renderer.development.js +0.4% +0.4% 534.42 KB 536.36 KB 115.15 KB 115.63 KB NODE_DEV
react-test-renderer.production.min.js 🔺+0.6% 🔺+0.5% 73.16 KB 73.59 KB 22.02 KB 22.12 KB NODE_PROD
ReactTestRenderer-dev.js +0.4% +0.4% 568.22 KB 570.26 KB 119.81 KB 120.29 KB FB_WWW_DEV

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom-server.browser.development.js 0.0% -0.0% 135.47 KB 135.47 KB 34.7 KB 34.7 KB UMD_DEV
react-dom-server.browser.production.min.js 0.0% -0.0% 20.01 KB 20.01 KB 7.41 KB 7.41 KB UMD_PROD
react-dom.profiling.min.js +0.3% +0.2% 126.62 KB 126.99 KB 39.54 KB 39.63 KB NODE_PROFILING
react-dom-unstable-fizz.browser.development.js 0.0% -0.1% 4.82 KB 4.82 KB 1.62 KB 1.62 KB UMD_DEV
ReactDOM-dev.js +0.2% +0.2% 964.32 KB 966.36 KB 213.76 KB 214.23 KB FB_WWW_DEV
react-dom-unstable-fizz.browser.production.min.js 0.0% -0.1% 1.2 KB 1.2 KB 706 B 705 B UMD_PROD
ReactDOM-prod.js 🔺+0.3% 🔺+0.2% 404.55 KB 405.67 KB 73.03 KB 73.2 KB FB_WWW_PROD
ReactDOM-profiling.js +0.3% +0.2% 415.37 KB 416.49 KB 74.87 KB 75.03 KB FB_WWW_PROFILING
react-dom-unstable-fizz.browser.production.min.js 0.0% -0.2% 1.02 KB 1.02 KB 618 B 617 B NODE_PROD
ReactDOMTesting-dev.js +0.2% +0.2% 918.72 KB 920.76 KB 204.54 KB 205.01 KB FB_WWW_DEV
ReactDOMTesting-prod.js 🔺+0.3% 🔺+0.2% 390.02 KB 391.14 KB 70.94 KB 71.11 KB FB_WWW_PROD
ReactDOMTesting-profiling.js +0.3% +0.2% 390.02 KB 391.14 KB 70.94 KB 71.11 KB FB_WWW_PROFILING
react-dom-server.browser.production.min.js 0.0% -0.0% 19.92 KB 19.92 KB 7.38 KB 7.38 KB NODE_PROD
react-dom.development.js +0.2% +0.2% 923.67 KB 925.67 KB 201.49 KB 201.94 KB UMD_DEV
react-dom.production.min.js 🔺+0.3% 🔺+0.3% 122.47 KB 122.84 KB 39.03 KB 39.14 KB UMD_PROD
ReactDOMForked-dev.js +0.2% +0.2% 964.32 KB 966.36 KB 213.76 KB 214.23 KB FB_WWW_DEV
react-dom.profiling.min.js +0.3% +0.2% 126.3 KB 126.66 KB 40.3 KB 40.39 KB UMD_PROFILING
ReactDOMForked-prod.js 🔺+0.3% 🔺+0.2% 404.55 KB 405.67 KB 73.03 KB 73.2 KB FB_WWW_PROD
react-dom.development.js +0.2% +0.2% 879.5 KB 881.44 KB 199.01 KB 199.48 KB NODE_DEV
ReactDOMForked-profiling.js +0.3% +0.2% 415.37 KB 416.49 KB 74.87 KB 75.03 KB FB_WWW_PROFILING
react-dom-unstable-fizz.node.development.js 0.0% -0.1% 5.09 KB 5.09 KB 1.68 KB 1.68 KB NODE_DEV
react-dom.production.min.js 🔺+0.3% 🔺+0.4% 122.64 KB 123.01 KB 38.25 KB 38.4 KB NODE_PROD
react-dom-unstable-native-dependencies.production.min.js 0.0% -0.1% 9.75 KB 9.75 KB 3.26 KB 3.25 KB NODE_PROD
react-dom-unstable-fizz.node.production.min.js 0.0% -0.1% 1.17 KB 1.17 KB 667 B 666 B NODE_PROD

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactART-dev.js +0.4% +0.4% 578.94 KB 580.98 KB 121.49 KB 121.97 KB FB_WWW_DEV
ReactART-prod.js 🔺+0.5% 🔺+0.5% 239.41 KB 240.67 KB 40.48 KB 40.67 KB FB_WWW_PROD
react-art.development.js +0.3% +0.3% 643 KB 645 KB 135.06 KB 135.52 KB UMD_DEV
react-art.production.min.js 🔺+0.4% 🔺+0.3% 108.89 KB 109.32 KB 32.92 KB 33.02 KB UMD_PROD
react-art.development.js +0.4% +0.4% 547.04 KB 548.97 KB 117.44 KB 117.91 KB NODE_DEV
react-art.production.min.js 🔺+0.6% 🔺+0.5% 73.88 KB 74.31 KB 22.11 KB 22.22 KB NODE_PROD

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-dev.js +0.3% +0.4% 648.8 KB 650.84 KB 139.14 KB 139.63 KB RN_FB_DEV
ReactNativeRenderer-prod.js 🔺+0.5% 🔺+0.4% 269.34 KB 270.59 KB 46.37 KB 46.56 KB RN_FB_PROD
ReactNativeRenderer-profiling.js +0.4% +0.4% 281.24 KB 282.49 KB 48.58 KB 48.77 KB RN_FB_PROFILING
ReactFabric-dev.js +0.3% +0.4% 628.1 KB 630.14 KB 134.37 KB 134.86 KB RN_OSS_DEV
ReactFabric-prod.js 🔺+0.5% 🔺+0.4% 261.17 KB 262.43 KB 44.84 KB 45.03 KB RN_OSS_PROD
ReactFabric-profiling.js +0.5% +0.4% 273.08 KB 274.33 KB 47.06 KB 47.25 KB RN_OSS_PROFILING
ReactFabric-dev.js +0.3% +0.4% 630.52 KB 632.56 KB 134.68 KB 135.18 KB RN_FB_DEV
ReactFabric-prod.js 🔺+0.5% 🔺+0.4% 261.33 KB 262.58 KB 44.87 KB 45.06 KB RN_FB_PROD
ReactNativeRenderer-dev.js +0.3% +0.4% 646.39 KB 648.43 KB 138.81 KB 139.3 KB RN_OSS_DEV
ReactFabric-profiling.js +0.5% +0.4% 273.23 KB 274.48 KB 47.09 KB 47.28 KB RN_FB_PROFILING
ReactNativeRenderer-prod.js 🔺+0.5% 🔺+0.4% 269.19 KB 270.44 KB 46.34 KB 46.53 KB RN_OSS_PROD
ReactNativeRenderer-profiling.js +0.4% +0.4% 281.1 KB 282.35 KB 48.55 KB 48.74 KB RN_OSS_PROFILING

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler-reflection.development.js 0.0% -0.0% 16.13 KB 16.13 KB 4.88 KB 4.88 KB NODE_DEV
react-reconciler.development.js +0.3% +0.4% 587.4 KB 589.34 KB 123.77 KB 124.24 KB NODE_DEV
react-reconciler.production.min.js 🔺+0.5% 🔺+0.6% 79.09 KB 79.47 KB 23.26 KB 23.4 KB NODE_PROD

ReactDOM: size: 0.0%, gzip: -0.0%

Size changes (experimental)

Generated by 🚫 dangerJS against 86da5ce

@sizebot
Copy link

@sizebot sizebot commented Mar 13, 2020

Details of bundled changes.

Comparing: a8f2165...86da5ce

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.production.min.js 🔺+0.3% 🔺+0.3% 118.51 KB 118.87 KB 37.18 KB 37.28 KB NODE_PROD
react-dom-test-utils.development.js 0.0% -0.0% 61.81 KB 61.81 KB 16.37 KB 16.37 KB UMD_DEV
ReactDOMTesting-profiling.js +0.3% +0.2% 401.99 KB 403.12 KB 72.84 KB 73 KB FB_WWW_PROFILING
react-dom-server.browser.production.min.js 0.0% -0.0% 19.46 KB 19.46 KB 7.3 KB 7.3 KB NODE_PROD
react-dom-unstable-fizz.browser.development.js 0.0% -0.1% 4.81 KB 4.81 KB 1.61 KB 1.61 KB UMD_DEV
react-dom.profiling.min.js +0.3% +0.2% 122.37 KB 122.72 KB 38.33 KB 38.41 KB NODE_PROFILING
react-dom-unstable-fizz.browser.production.min.js 0.0% -0.1% 1.19 KB 1.19 KB 698 B 697 B UMD_PROD
react-dom-server.node.development.js 0.0% -0.0% 128.22 KB 128.22 KB 34.32 KB 34.32 KB NODE_DEV
react-dom-server.node.production.min.js 0.0% -0.0% 19.87 KB 19.87 KB 7.45 KB 7.44 KB NODE_PROD
ReactDOMForked-dev.js +0.2% +0.2% 992.2 KB 994.24 KB 220.14 KB 220.61 KB FB_WWW_DEV
ReactDOMForked-prod.js 🔺+0.3% 🔺+0.2% 417.61 KB 418.73 KB 75.05 KB 75.21 KB FB_WWW_PROD
react-dom.development.js +0.2% +0.2% 893.84 KB 895.84 KB 196.19 KB 196.65 KB UMD_DEV
ReactDOMForked-profiling.js +0.3% +0.2% 428.49 KB 429.61 KB 76.93 KB 77.1 KB FB_WWW_PROFILING
react-dom-unstable-fizz.node.production.min.js 0.0% -0.2% 1.16 KB 1.16 KB 660 B 659 B NODE_PROD
react-dom.production.min.js 🔺+0.3% 🔺+0.3% 118.41 KB 118.76 KB 37.95 KB 38.06 KB UMD_PROD
react-dom-server.browser.production.min.js 0.0% -0.0% 19.55 KB 19.55 KB 7.33 KB 7.32 KB UMD_PROD
react-dom.profiling.min.js +0.3% +0.4% 122.12 KB 122.47 KB 39.08 KB 39.23 KB UMD_PROFILING
ReactDOMTesting-dev.js +0.2% +0.2% 945.27 KB 947.31 KB 210.38 KB 210.86 KB FB_WWW_DEV
react-dom.development.js +0.2% +0.2% 850.91 KB 852.85 KB 193.78 KB 194.25 KB NODE_DEV
ReactDOMTesting-prod.js 🔺+0.3% 🔺+0.2% 401.99 KB 403.12 KB 72.84 KB 73 KB FB_WWW_PROD
ReactDOM-dev.js +0.2% +0.2% 992.2 KB 994.24 KB 220.14 KB 220.61 KB FB_WWW_DEV
ReactDOM-prod.js 🔺+0.3% 🔺+0.2% 417.61 KB 418.73 KB 75.05 KB 75.21 KB FB_WWW_PROD
react-dom-test-utils.development.js 0.0% -0.0% 57.18 KB 57.18 KB 15.73 KB 15.73 KB NODE_DEV
react-dom-unstable-fizz.browser.development.js 0.0% -0.1% 4.33 KB 4.33 KB 1.51 KB 1.51 KB NODE_DEV
ReactDOM-profiling.js +0.3% +0.2% 428.49 KB 429.61 KB 76.93 KB 77.1 KB FB_WWW_PROFILING
react-dom-unstable-fizz.browser.production.min.js 0.0% -0.2% 1 KB 1 KB 610 B 609 B NODE_PROD
react-dom-unstable-native-dependencies.production.min.js 0.0% -0.0% 9.73 KB 9.73 KB 3.25 KB 3.25 KB NODE_PROD

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-dev.js +0.3% +0.4% 646.38 KB 648.42 KB 138.81 KB 139.29 KB RN_OSS_DEV
ReactNativeRenderer-prod.js 🔺+0.5% 🔺+0.4% 269.18 KB 270.43 KB 46.33 KB 46.53 KB RN_OSS_PROD
ReactNativeRenderer-profiling.js +0.4% +0.4% 281.08 KB 282.34 KB 48.54 KB 48.73 KB RN_OSS_PROFILING
ReactFabric-dev.js +0.3% +0.4% 628.09 KB 630.13 KB 134.37 KB 134.86 KB RN_OSS_DEV
ReactFabric-prod.js 🔺+0.5% 🔺+0.4% 261.16 KB 262.42 KB 44.83 KB 45.02 KB RN_OSS_PROD
ReactFabric-profiling.js +0.5% +0.4% 273.07 KB 274.32 KB 47.05 KB 47.24 KB RN_OSS_PROFILING

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.development.js +0.4% +0.4% 527.56 KB 529.5 KB 113.7 KB 114.16 KB NODE_DEV
react-art.production.min.js 🔺+0.6% 🔺+0.4% 71.32 KB 71.75 KB 21.48 KB 21.58 KB NODE_PROD
ReactART-dev.js +0.3% +0.4% 589.01 KB 591.05 KB 123.49 KB 123.95 KB FB_WWW_DEV
ReactART-prod.js 🔺+0.5% 🔺+0.5% 247.01 KB 248.27 KB 41.75 KB 41.95 KB FB_WWW_PROD
react-art.development.js +0.3% +0.3% 622.73 KB 624.73 KB 131.38 KB 131.84 KB UMD_DEV
react-art.production.min.js 🔺+0.4% 🔺+0.2% 106.28 KB 106.71 KB 32.26 KB 32.32 KB UMD_PROD

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer-shallow.development.js 0.0% 0.0% 38.62 KB 38.62 KB 9.39 KB 9.39 KB UMD_DEV
react-test-renderer-shallow.production.min.js 0.0% 0.0% 12.73 KB 12.73 KB 3.96 KB 3.97 KB UMD_PROD
react-test-renderer.development.js +0.4% +0.4% 534.39 KB 536.33 KB 115.14 KB 115.62 KB NODE_DEV
react-test-renderer.production.min.js 🔺+0.6% 🔺+0.5% 73.13 KB 73.56 KB 22 KB 22.1 KB NODE_PROD
ReactTestRenderer-dev.js +0.4% +0.4% 568.21 KB 570.25 KB 119.8 KB 120.28 KB FB_WWW_DEV
react-test-renderer.development.js +0.4% +0.4% 560.5 KB 562.49 KB 116.58 KB 117.05 KB UMD_DEV
react-test-renderer.production.min.js 🔺+0.6% 🔺+0.5% 73.3 KB 73.73 KB 22.31 KB 22.41 KB UMD_PROD

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler.development.js +0.3% +0.4% 565.56 KB 567.5 KB 119.53 KB 119.99 KB NODE_DEV
react-reconciler.production.min.js 🔺+0.5% 🔺+0.5% 76.13 KB 76.5 KB 22.55 KB 22.65 KB NODE_PROD

Size changes (stable)

Generated by 🚫 dangerJS against 86da5ce

@bvaughn bvaughn force-pushed the bvaughn:useMutableSource-bugfix branch from bc57bac to 7af270e Mar 17, 2020
// It's important to ensure that this update is included with that one.
// TODO This sucks; what's the right way of doing this?
runWithPriority(UserBlockingPriority, () => {
setSnapshot(maybeNewSnapshot);

This comment has been minimized.

@bvaughn

bvaughn Mar 17, 2020
Author Contributor

As I mentioned in this comment, I think what I need to do here is schedule a state update with the same expiration time as the one that might be pending due to a mutation. (Either that or manually replace the pending update with this one, but that seemed a little shady too.)

Added this "fix" in the meanwhile, for discussion purposes. 😄

@bvaughn bvaughn force-pushed the bvaughn:useMutableSource-bugfix branch 3 times, most recently from f5e7746 to fde2923 Mar 17, 2020
@bvaughn
Copy link
Contributor Author

@bvaughn bvaughn commented Mar 31, 2020

I know you've been busy @acdlite, but friendly ping on this 😄

Dave was talking to me this afternoon about a bug he's seeing with Recoil and I'd like to at least sync this so we can see if the fix helped or not.

setSnapshot(() => {
throw error;
});
latestSetSnapshot(

This comment has been minimized.

@acdlite

acdlite Mar 31, 2020
Member

I think you need to do the same markRootExpiredAtTime thing during initial mount and when resubscribing, too, to avoid a momentary flicker.

This comment has been minimized.

@bvaughn

bvaughn Mar 31, 2020
Author Contributor

Hm...do we?

A change firing between render and subscription means that we missed an update, but we have other checks in place to avoid actually tearing between other components that read from the source.

This comment has been minimized.

@bvaughn

bvaughn Mar 31, 2020
Author Contributor

Put another way, the reason we added it where we did was because there's a potential tear- but in the subscribe-on-commit case, there's not a tear, just a potential missed update. Also at that point, we would have already shown the older value because we subscribe in a passive effect.

Unless I'm misunderstanding what you're suggesting?

This comment has been minimized.

@bvaughn

bvaughn Apr 1, 2020
Author Contributor

Follow up on the Dave /Recoil comment: Been stepping through that today, and it looks like the Recoil getter is also a setter, which is causing a loop. (In other words I think it's a Recoil problem, not a uMS problem.)

@bvaughn
Copy link
Contributor Author

@bvaughn bvaughn commented Mar 31, 2020

I think this test captures the mount/flicker case we talked about:

it("blah", async () => {
  const source = createSource("one");
  const mutableSource = createMutableSource(source);

  let committedA = null;
  let committedB = null;

  const onRender = () => {
    if (committedB !== null) {
      expect(committedA).toBe(committedB);
    }
  };

  function ComponentA() {
    const snapshot = useMutableSource(
      mutableSource,
      defaultGetSnapshot,
      defaultSubscribe
    );
    Scheduler.unstable_yieldValue(`a:${snapshot}`);
    React.useEffect(() => {
      committedA = snapshot;
    }, [snapshot]);
    return <div>{`a:${snapshot}`}</div>;
  }
  function ComponentB() {
    const snapshot = useMutableSource(
      mutableSource,
      defaultGetSnapshot,
      defaultSubscribe
    );
    Scheduler.unstable_yieldValue(`b:${snapshot}`);
    React.useEffect(() => {
      committedB = snapshot;
    }, [snapshot]);
    return <div>{`b:${snapshot}`}</div>;
  }

  // Mount ComponentA with data version 1
  act(() => {
    ReactNoop.render(
      <React.Profiler id="root" onRender={onRender}>
        <ComponentA />
      </React.Profiler>,
      () => Scheduler.unstable_yieldValue("Sync effect")
    );
  });
  expect(Scheduler).toHaveYielded(["a:one", "Sync effect"]);
  expect(source.listenerCount).toBe(1);

  // Mount ComponentB with version 1 (but don't commit it)
  act(() => {
    ReactNoop.render(
      <React.Profiler id="root" onRender={onRender}>
        <ComponentA />
        <ComponentB />
      </React.Profiler>,
      () => Scheduler.unstable_yieldValue("Sync effect")
    );
    expect(Scheduler).toFlushAndYieldThrough(["a:one", "b:one", "Sync effect"]);
    expect(source.listenerCount).toBe(1);

    // Mutate -> schedule update for ComponentA
    Scheduler.unstable_runWithPriority(Scheduler.unstable_IdlePriority, () => {
      source.value = "two";
    });

    // Commit ComponentB -> notice the change and schedule an update for ComponentB
    expect(Scheduler).toFlushAndYield(["a:two", "b:two"]);
    expect(source.listenerCount).toBe(2);
  });
});
@bvaughn
Copy link
Contributor Author

@bvaughn bvaughn commented Mar 31, 2020

I think the problem with what we talked about - only storing the lowest priority (highest expiration time) pending update - is that we are trying to use a single value for two things:

  1. Flush all pending work for a root in certain cases (using markRootExpiredAtTime). The lowest priority works well for this.
  2. Determine if it's safe to read from a source when we haven't subscribed yet or when getSnapshot changes (by comparing the pending time to renderExpirationTime). The lowest priority doesn't work for this because it can give us a false positive. (See the currently failing test for an example.)

To be clear, I think the tests only happened to work before. I think this was a potential problem then too.

For now I'll push the new flicker test, with the fix, but I'm not sure how to work around this second issue.

@bvaughn bvaughn force-pushed the bvaughn:useMutableSource-bugfix branch from fde2923 to 1383900 Mar 31, 2020
@acdlite
Copy link
Member

@acdlite acdlite commented Apr 1, 2020

For the other types of pending work, we track the range:

// The earliest pending expiration time that exists in the tree
firstPendingTime: ExpirationTime,
// The latest pending expiration time that exists in the tree
lastPendingTime: ExpirationTime,
// The earliest suspended expiration time that exists in the tree
firstSuspendedTime: ExpirationTime,
// The latest suspended expiration time that exists in the tree
lastSuspendedTime: ExpirationTime,

We can do the same thing here

@bvaughn
Copy link
Contributor Author

@bvaughn bvaughn commented Apr 1, 2020

For the other types of pending work, we track the range

Yeah. That's what I ended up thinking too. I'll take a pass at it soon.

FWIW, I did try doing a naive version of this yesterday (just tracking lowest and highest priority) and it didn't seem sufficient, but I'll take another pass. Got distracted by some other reviews.

@bvaughn
Copy link
Contributor Author

@bvaughn bvaughn commented Apr 2, 2020

Gah. I had a small, stupid mistake in my first try at the separate expiration times, and it just took me 2 hours of staring at the code to spot it.

@bvaughn
Copy link
Contributor Author

@bvaughn bvaughn commented Apr 2, 2020

Ok back to you @acdlite

@bvaughn bvaughn force-pushed the bvaughn:useMutableSource-bugfix branch from 1383900 to 99a8fc7 Apr 2, 2020
// We missed a mutation before committing.
// It's possible that other components using this source also have pending updates scheduled.
// In that case, we should ensure they all commit together.
markRootExpiredAtTime(root, getLastPendingExpirationTime(root));

This comment has been minimized.

@acdlite

acdlite Apr 2, 2020
Member

Need to mark this new setSnapshot call as a pending mutation, too, right before marking it as expired.

This comment has been minimized.

@bvaughn

bvaughn Apr 2, 2020
Author Contributor

This change makes sense conceptually, but it breaks the most recent test case I added.

In this new test I...

  1. Mount and flush component A with data version 1.
  2. Mount component B with data version 1 (but flush passive effects).
  3. Mutate the data (which schedules an update for already mounted component A.
  4. Flush passive effects (which also schedules an update for the newly mounted component B).
  5. Verify there was no tearing.

Before this change, both components A and B rendered together after we flushed passive effects.

After this change, component B renders and commits, then component A (and we tear in between). This is because- with the change in place, we're marking the root as expired with a higher priority (e.g. 1073741296 instead of 2/idle).

I think this indicates that we should be marking root as expired with the first expiration time rather than the last in this case? At least, doing that fixes this test (and also the other two failing tests that were tearing before). This also means that we aren't directly using last expiration time for anything at this point (although indicate it determines when we reset first expiration time in clearPendingUpdates).

This comment has been minimized.

@bvaughn

bvaughn Apr 2, 2020
Author Contributor

I'm going to push this change as a separate commit (rather than squashing) just to give you the opportunity to review it more closely: 7c9181f

Back to you @acdlite

…onger in use "first" time value
@bvaughn bvaughn force-pushed the bvaughn:useMutableSource-bugfix branch from 7c9181f to db511f4 Apr 3, 2020
@bvaughn
Copy link
Contributor Author

@bvaughn bvaughn commented Apr 3, 2020

This is flipped.

Ah. Assuming you just mean in this one place (and not everywhere) then I think tracking the "first" time may not be necessary. (Same thing as I said with my comment above, but with the labels swapped since I still can't make my brain think of the expirationTime with the lowest value as being the "last" time.)

Going to push this change as a separate commit as well, to make it easier for you to review in isolation: db511f4

@acdlite
acdlite approved these changes Apr 3, 2020
Copy link
Member

@acdlite acdlite left a comment

Just one last nit and I think this is good

) {
if (didGetSnapshotChange) {

This comment has been minimized.

@acdlite

acdlite Apr 3, 2020
Member

Can remove this nested check. Need to drop updates from new sources, too.

This comment has been minimized.

@bvaughn

bvaughn Apr 3, 2020
Author Contributor

Yeah, I wasn't sure about that... I'll remove it 😄

Thanks for the review!

This comment has been minimized.

@bvaughn

bvaughn Apr 3, 2020
Author Contributor

Hm, I think removing this check actually breaks some subtle assumptions about the effects and when we need to update the getSnapshot/setSnapshot refs. I think it would cause us to drop updates for mutations after a source changed... need to think about it a bit.

This comment has been minimized.

@acdlite

acdlite Apr 3, 2020
Member

I think it works because when source or subscribe changes, you're going to unsubscribe/resubscribe.

When getSnapshot changes, you don't resubscribe but you do track the latest value with a ref. Which I think of as an optimization for resubscribing; you're still "changing" the subscription in both cases. So they are the same thing except for what we do in the commit phase. (For example, I think all your tests would pass if you removed the effect and ref that tracks getSnapshot and instead added getSnapshot as a dependency to the resubscribe hook.)

This comment has been minimized.

@bvaughn

bvaughn Apr 3, 2020
Author Contributor

Ah, I see how to fix it. Will let me simplify things a bit too...

This comment has been minimized.

@bvaughn

bvaughn Apr 3, 2020
Author Contributor

Okay, here's what 86da5ce did:

Recreating the queue and dispatch if source or subscribe changed too (not just getSnapshot) meant that the we needed to update refs.setSnapshot in that case too. (Before that effect was only run when getSnapshot changed.)

So I added source or subscribe as dependencies to the first effect so it would update the dispatch ref. Since we're now also running this effect whenever we resubscribe, I was also able to consolidate the check for a changed snapshot between render and passive effects into just the first effect (rather than both).

This comment has been minimized.

@bvaughn

bvaughn Apr 3, 2020
Author Contributor

Weird. I didn't see any of your interleaved commits until just now, after I left mine. Heh. Didn't mean to seem like I was ignoring you.

For example, I think all your tests would pass if you removed the effect and ref that tracks getSnapshot and instead added getSnapshot as a dependency to the resubscribe hook.

This requires us to over-subscribe though, (since we can't share that effect without running the previous cleanup function and unsubscribing). Once we replace useEffect with our own custom pushed effect, we can optimize this though.

This comment has been minimized.

@bvaughn

bvaughn Apr 3, 2020
Author Contributor

To be clear, I'm trying to avoid re-subscribing unless actually necessary (not just convenient) because subscriptions can be expensive for some of useMutableSource's users. That was the whole reason to use a ref in the first place for get/set snapshot.

This comment has been minimized.

@acdlite

acdlite Apr 3, 2020
Member

Right, I can see why you did it this way. I think we might want to revisit the trade off in the future, though, since changing getSnapshot is expensive regardless because it leads to deopts. So it needs to be relatively stable. (I am kinda worried about this, since it's easy for users to mess up.) And if we assume it's relatively stable, then resubscribing might not be so bad.

This comment has been minimized.

@bvaughn

bvaughn Apr 3, 2020
Author Contributor

since changing getSnapshot is expensive regardless because it leads to deopts. So it needs to be relatively stable.

Yeah, absolutely. Maybe there's something we could do to detect when people are passing inline functions, so we could warn.

// In that case, the subscription below will have cloesd over the previous function,
// so we use a ref to ensure that handleChange() always has the latest version.
// but this hook recreates the queue in certain cases to avoid updates from stale sources.
// handleChange() below needs to reference the dispatch function without re-subscribing,

This comment has been minimized.

@acdlite

acdlite Apr 3, 2020
Member

I think if you go with this approach then you don't have to track getSnapshot or setSnapshot at all. Can use the closed over values, since getSnapshot is now a dep, and setSnapshot only changes when one of the other deps does.

This comment has been minimized.

@acdlite

acdlite Apr 3, 2020
Member

Edit: Got confused, thought you had removed the getSnapshot optimization. Never mind.

This comment has been minimized.

@bvaughn

bvaughn Apr 3, 2020
Author Contributor

No worries. This is something we can further optimize when we replace the two composed effects with our own pushed effect. Then we can just use one single effect and selectively unsubscribe/subscribe only when necessary.

Sounds like that will be a fun little cleanup refactor anyway. 😄

!is(prevSource, source) ||
!is(prevSubscribe, subscribe) ||
!is(prevGetSnapshot, getSnapshot)
!is(prevSubscribe, subscribe)

This comment has been minimized.

@acdlite

acdlite Apr 3, 2020
Member

This is neat that this is now the same condition that the effect hook uses when comparing deps. In the future we could "cheat" and read the deps off the effect hook instead of using a ref.

@bvaughn bvaughn merged commit f312a3f into facebook:master Apr 3, 2020
31 checks passed
31 checks passed
ci/circleci: NODE_ENV_production_yarn_test_www Your tests passed on CircleCI!
Details
ci/circleci: NODE_ENV_production_yarn_test_www_variant Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_NODE_ENV_production_yarn_test_www Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_NODE_ENV_production_yarn_test_www_variant Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_build Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test_build Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test_build_prod Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test_dom_fixtures Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test_persistent Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test_prod Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test_www Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test_www_variant Your tests passed on CircleCI!
Details
ci/circleci: build_devtools_and_process_artifacts Your tests passed on CircleCI!
Details
ci/circleci: process_artifacts Your tests passed on CircleCI!
Details
ci/circleci: process_artifacts_experimental Your tests passed on CircleCI!
Details
ci/circleci: setup Your tests passed on CircleCI!
Details
ci/circleci: sizebot_experimental Your tests passed on CircleCI!
Details
ci/circleci: sizebot_stable Your tests passed on CircleCI!
Details
ci/circleci: yarn_build Your tests passed on CircleCI!
Details
ci/circleci: yarn_flow Your tests passed on CircleCI!
Details
ci/circleci: yarn_lint Your tests passed on CircleCI!
Details
ci/circleci: yarn_lint_build Your tests passed on CircleCI!
Details
ci/circleci: yarn_test Your tests passed on CircleCI!
Details
ci/circleci: yarn_test_build Your tests passed on CircleCI!
Details
ci/circleci: yarn_test_build_devtools Your tests passed on CircleCI!
Details
ci/circleci: yarn_test_build_prod Your tests passed on CircleCI!
Details
ci/circleci: yarn_test_prod Your tests passed on CircleCI!
Details
ci/circleci: yarn_test_www Your tests passed on CircleCI!
Details
ci/circleci: yarn_test_www_variant Your tests passed on CircleCI!
Details
ci/codesandbox Building packages succeeded.
Details
@bvaughn bvaughn deleted the bvaughn:useMutableSource-bugfix branch Apr 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.