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 March 13, 2020 00:48
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Mar 13, 2020
@codesandbox-ci
Copy link

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-ci
Copy link

codesandbox-ci 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 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 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

// 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);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 useMutableSource-bugfix branch 3 times, most recently from f5e7746 to fde2923 Compare March 20, 2020 16:20
@bvaughn
Copy link
Contributor Author

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(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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 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.

@acdlite
Copy link
Collaborator

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 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 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 commented Apr 2, 2020

Ok back to you @acdlite

// 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));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@bvaughn
Copy link
Contributor Author

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

Copy link
Collaborator

@acdlite acdlite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one last nit and I think this is good

) {
if (didGetSnapshotChange) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Thanks for the review!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@bvaughn bvaughn Apr 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants