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

Migrate useDeferredValue and useTransition #17058

Merged
merged 11 commits into from
Oct 18, 2019

Conversation

lunaruan
Copy link
Contributor

@lunaruan lunaruan commented Oct 10, 2019

Migrated useDeferredValue and useTransition from Facebook's www repo into ReactFiberHooks.

@lunaruan lunaruan changed the title Migrate useSuspenseDeferredValue and useSuspenseTransition Migrate useSuspenseDeferredValue and useSuspenseTransition Oct 10, 2019
@sizebot
Copy link

sizebot commented Oct 10, 2019

React: size: 0.0%, gzip: 0.0%

ReactDOM: size: 🔺+0.5%, gzip: 🔺+0.3%

Details of bundled changes.

Comparing: abedf17...7b7db2d

react

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react.development.js +0.4% +0.2% 117.67 KB 118.08 KB 29.87 KB 29.94 KB UMD_DEV
react.production.min.js 0.0% 0.0% 12.43 KB 12.43 KB 4.9 KB 4.9 KB UMD_PROD
react.development.js +0.6% +0.3% 72.99 KB 73.4 KB 19.18 KB 19.25 KB NODE_DEV
React-dev.js +0.6% +0.4% 70.55 KB 70.97 KB 18.18 KB 18.25 KB FB_WWW_DEV
React-prod.js 0.0% -0.0% 17.49 KB 17.49 KB 4.58 KB 4.57 KB FB_WWW_PROD
React-profiling.js 0.0% -0.0% 17.49 KB 17.49 KB 4.58 KB 4.57 KB FB_WWW_PROFILING

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.profiling.min.js +0.7% +0.6% 121.33 KB 122.22 KB 38.23 KB 38.44 KB NODE_PROFILING
react-dom-server.browser.development.js +0.2% +0.2% 140.87 KB 141.21 KB 36.91 KB 36.99 KB UMD_DEV
ReactDOM-dev.js +0.5% +0.2% 975.81 KB 980.21 KB 216.43 KB 216.9 KB FB_WWW_DEV
react-dom-server.browser.production.min.js 🔺+0.5% 🔺+0.3% 19.86 KB 19.96 KB 7.37 KB 7.4 KB UMD_PROD
react-dom-test-utils.production.min.js 0.0% 🔺+0.1% 11.19 KB 11.19 KB 4.16 KB 4.16 KB UMD_PROD
react-dom-test-utils.production.min.js 0.0% 🔺+0.1% 10.96 KB 10.96 KB 4.09 KB 4.09 KB NODE_PROD
react-dom.development.js +0.4% +0.2% 951.36 KB 955.58 KB 215.29 KB 215.71 KB UMD_DEV
react-dom.production.min.js 🔺+0.7% 🔺+0.6% 117.58 KB 118.43 KB 37.9 KB 38.13 KB UMD_PROD
react-dom.profiling.min.js +0.7% +0.6% 121.11 KB 121.97 KB 38.92 KB 39.15 KB UMD_PROFILING
react-dom.development.js +0.5% +0.2% 945.41 KB 949.66 KB 213.73 KB 214.15 KB NODE_DEV
react-dom-server.node.development.js +0.2% +0.2% 137.88 KB 138.23 KB 36.15 KB 36.22 KB NODE_DEV
react-dom.production.min.js 🔺+0.8% 🔺+0.5% 117.69 KB 118.58 KB 37.21 KB 37.4 KB NODE_PROD
react-dom-server.node.production.min.js 🔺+0.5% 🔺+0.4% 20.2 KB 20.3 KB 7.52 KB 7.55 KB NODE_PROD
ReactDOM-prod.js 🔺+0.7% 🔺+0.5% 398.63 KB 401.56 KB 72.74 KB 73.13 KB FB_WWW_PROD
ReactDOM-profiling.js +0.7% +0.5% 399.44 KB 402.37 KB 73.27 KB 73.65 KB FB_WWW_PROFILING
react-dom-server.browser.development.js +0.3% +0.2% 136.8 KB 137.14 KB 35.92 KB 36 KB NODE_DEV
react-dom-server.browser.production.min.js 🔺+0.5% 🔺+0.4% 19.79 KB 19.89 KB 7.36 KB 7.39 KB NODE_PROD
ReactDOMServer-dev.js +0.2% +0.2% 140.69 KB 141.03 KB 35.48 KB 35.56 KB FB_WWW_DEV
ReactDOMServer-prod.js 🔺+0.6% 🔺+0.7% 48.24 KB 48.53 KB 11.04 KB 11.12 KB FB_WWW_PROD
react-dom-unstable-fizz.node.development.js 0.0% +0.1% 3.87 KB 3.87 KB 1.51 KB 1.51 KB NODE_DEV
react-dom-unstable-fizz.node.production.min.js 0.0% 🔺+0.2% 1.1 KB 1.1 KB 666 B 667 B NODE_PROD

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.development.js +0.6% +0.3% 669.47 KB 673.69 KB 145.54 KB 145.97 KB UMD_DEV
react-art.production.min.js 🔺+0.8% 🔺+0.8% 104 KB 104.85 KB 31.63 KB 31.88 KB UMD_PROD
react-art.development.js +0.7% +0.3% 600.1 KB 604.36 KB 128.13 KB 128.55 KB NODE_DEV
react-art.production.min.js 🔺+1.3% 🔺+0.9% 69.01 KB 69.9 KB 20.92 KB 21.1 KB NODE_PROD
ReactART-dev.js +0.7% +0.4% 614.81 KB 619.21 KB 127.96 KB 128.43 KB FB_WWW_DEV
ReactART-prod.js 🔺+1.3% 🔺+0.9% 231.72 KB 234.65 KB 39.13 KB 39.49 KB FB_WWW_PROD

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-prod.js 🔺+1.1% 🔺+0.8% 275.97 KB 278.9 KB 47.19 KB 47.58 KB RN_OSS_PROD
ReactNativeRenderer-profiling.js +1.0% +0.8% 285.51 KB 288.43 KB 48.93 KB 49.3 KB RN_OSS_PROFILING
ReactFabric-prod.js 🔺+1.1% 🔺+0.8% 267.27 KB 270.2 KB 45.77 KB 46.16 KB RN_OSS_PROD
ReactFabric-profiling.js +1.1% +0.8% 277.92 KB 280.85 KB 47.64 KB 48.01 KB RN_OSS_PROFILING
ReactFabric-dev.js +0.6% +0.3% 751.69 KB 756.09 KB 159.07 KB 159.51 KB RN_FB_DEV
ReactFabric-prod.js 🔺+1.1% 🔺+0.8% 267.28 KB 270.2 KB 45.78 KB 46.17 KB RN_FB_PROD
ReactNativeRenderer-dev.js +0.6% +0.3% 747.21 KB 751.61 KB 158.37 KB 158.82 KB RN_OSS_DEV
ReactFabric-profiling.js +1.1% +0.8% 277.92 KB 280.84 KB 47.65 KB 48.02 KB RN_FB_PROFILING
ReactNativeRenderer-dev.js +0.6% +0.3% 747.38 KB 751.77 KB 158.44 KB 158.89 KB RN_FB_DEV
ReactNativeRenderer-prod.js 🔺+1.1% 🔺+0.8% 275.97 KB 278.9 KB 47.2 KB 47.59 KB RN_FB_PROD
ReactNativeRenderer-profiling.js +1.0% +0.8% 285.5 KB 288.42 KB 48.93 KB 49.31 KB RN_FB_PROFILING
ReactFabric-dev.js +0.6% +0.3% 751.52 KB 755.92 KB 158.99 KB 159.44 KB RN_OSS_DEV

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactTestRenderer-dev.js +0.7% +0.4% 626.08 KB 630.49 KB 130.39 KB 130.85 KB FB_WWW_DEV
react-test-renderer-shallow.development.js +1.8% +1.3% 38.96 KB 39.67 KB 9.93 KB 10.06 KB UMD_DEV
react-test-renderer-shallow.production.min.js 🔺+1.5% 🔺+1.0% 11.45 KB 11.63 KB 3.54 KB 3.57 KB UMD_PROD
react-test-renderer-shallow.development.js +2.2% +1.6% 32.92 KB 33.63 KB 8.47 KB 8.6 KB NODE_DEV
react-test-renderer-shallow.production.min.js 🔺+1.5% 🔺+1.2% 11.6 KB 11.77 KB 3.64 KB 3.68 KB NODE_PROD
react-test-renderer.development.js +0.7% +0.3% 613.66 KB 617.92 KB 130.97 KB 131.41 KB UMD_DEV
react-test-renderer.production.min.js 🔺+1.2% 🔺+1.2% 70.88 KB 71.76 KB 21.69 KB 21.95 KB UMD_PROD
ReactShallowRenderer-dev.js +2.3% +1.7% 34.04 KB 34.84 KB 8.38 KB 8.52 KB FB_WWW_DEV
react-test-renderer.development.js +0.7% +0.3% 608.93 KB 613.19 KB 129.77 KB 130.21 KB NODE_DEV
react-test-renderer.production.min.js 🔺+1.3% 🔺+0.9% 70.58 KB 71.47 KB 21.38 KB 21.58 KB NODE_PROD

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler.development.js +0.7% +0.3% 601.55 KB 605.81 KB 127.46 KB 127.89 KB NODE_DEV
react-reconciler.production.min.js 🔺+1.2% 🔺+0.9% 71.79 KB 72.68 KB 21.25 KB 21.44 KB NODE_PROD
react-reconciler-reflection.production.min.js 0.0% 🔺+0.1% 2.88 KB 2.88 KB 1.25 KB 1.25 KB NODE_PROD
react-reconciler-persistent.development.js +0.7% +0.3% 598.73 KB 602.98 KB 126.27 KB 126.7 KB NODE_DEV
react-reconciler-persistent.production.min.js 🔺+1.2% 🔺+0.9% 71.8 KB 72.69 KB 21.26 KB 21.45 KB NODE_PROD

react-debug-tools

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-debug-tools.development.js +2.2% +1.2% 19.71 KB 20.14 KB 5.8 KB 5.87 KB NODE_DEV
react-debug-tools.production.min.js 🔺+3.6% 🔺+1.7% 6.01 KB 6.22 KB 2.41 KB 2.45 KB NODE_PROD

Generated by 🚫 dangerJS against 7b7db2d

@lunaruan lunaruan removed the request for review from threepointone October 10, 2019 01:42
@sebmarkbage
Copy link
Collaborator

As part of this migration I intended to drop the “Suspense” part. Let’s just call them useDeferredValue and useTransition.

The Suspense part was to bring them into a brand but going forward, it’s just going to be annoying to have the longer name.

It’s also a misnomer since given this set up, this is the primary API to call into the Concurrent Mode APIs even if you don’t use Suspense at all.

Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

The DevTools bits (ReactDebugHooks) look good. Let's be certain to coordinate an FB DevTools sync once this PR lands before we replace the userland hooks with React ones.

@lunaruan lunaruan changed the title Migrate useSuspenseDeferredValue and useSuspenseTransition Migrate useDeferredValue and useTransition Oct 10, 2019
@sebmarkbage
Copy link
Collaborator

You can probably update this warning now.

'Refer to the documentation for useSuspenseTransition to learn how ' +
'to implement this pattern.',
// TODO: Add link to React docs with more information, once it exists

@bvaughn
Copy link
Contributor

bvaughn commented Oct 11, 2019

We'll also need an accompanying PR to update the hooks API docs:
https://reactjs.org/docs/hooks-reference.html


ReactFeatureFlags = require('shared/ReactFeatureFlags');
ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false;
ReactFeatureFlags.enableSchedulerTracing = true;
ReactFeatureFlags.flushSuspenseFallbacksInTests = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this necessary? Can we rewrite the test somehow? I can pair with you on this if needed

@@ -27,6 +27,7 @@ describe('ReactHooks', () => {

ReactFeatureFlags = require('shared/ReactFeatureFlags');
ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false;
ReactFeatureFlags.enableStableConcurrentModeAPIs = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you rebase on master, you can wrap the concurrent tests with an __EXPERIMENTAL__ check instead.

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.

Looks good! Go ahead and merge once you address the feedback

Comment on lines 2152 to 2155
'A',
'B',
'Suspend! [B]',
'Loading',
Copy link
Contributor

Choose a reason for hiding this comment

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

I... don't understand the purpose of this (why 'A' is in the yields). deferredText will finish the current commit with the old value, and then update itself in a separate commit, but why? Doesn't <Suspense> already do that "for free"?

Copy link
Collaborator

@sebmarkbage sebmarkbage Oct 17, 2019

Choose a reason for hiding this comment

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

The yields are not important. The output is important. If it was using Suspense, the output would be [span('B'), span('Loading')] which isn't desirable here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd expect Suspense to hold off on committing the subtree at all (so it'd stay showing span('A')) until the expirationTime is reached.

...though I guess act() forces an expirationTime flush.

Copy link
Collaborator

@sebmarkbage sebmarkbage Oct 17, 2019

Choose a reason for hiding this comment

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

In that case we couldn't commit the outer A so it would be [span('A'), span('A')]. We have another way to do that which is what useTransition is for.

We never commit anything in an inconsistent state by default. So either we commit A in both slots or B in both slots. Early explorations included ways of leaving some trees inconsistent but the downstream effects on that in a reactive system was too difficult to reason about. E.g. imaging you also switched to Dark Mode in a Context at the same time.

useDeferredValue is a limited form of explicitly opting in to inconsistency for a particular value while keeping other values consistent.

ReactCurrentBatchConfig.suspense = config === undefined ? null : config;
try {
setPending(false);
callback();
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the callback run as a potentially separate commit, or does being inside Scheduler.next prevent non-explicit "unbatching"?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The intention here is to:

  1. Set pending false at high priority
  2. Batch setting pending to false and doing the actual state change at lower priority

As a result, you'll see pending immediately turn true, but then it turn false together with the actual state change you wanted to do.

},
[config, isPending],
);
return [startTransition, isPending];
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the only built-in hook producing a pair which returns a function as the first, instead of the second element. This will take some education.

Copy link
Contributor

Choose a reason for hiding this comment

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

That does seem odd. Should it be flipped?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The first argument is useful without the second but not the inverse.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn’t we nudge you to use the busy return value tho?

} finally {
ReactCurrentBatchConfig.suspense = previousConfig;
}
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we not pass the TimeoutConfig to Scheduler.unstable_next in the mount case, when we do in the update case?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice catch. unstable_next doesn't accept a second argument. It must have been a copypasta from withSuspenseConfig.

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, it was accidental copy/paste. Deleted it! :)

} finally {
ReactCurrentBatchConfig.suspense = previousConfig;
}
}, config);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This config doesn't seem right. Probably a copy paste error.

@sebmarkbage
Copy link
Collaborator

I wonder if we should remove withSuspenseConfig. It has the benefit that it can operate standalone from a specific component which might be useful in edge cases but we don't plan on documenting it early on.

If we do, then the question becomes whether we should move ReactCurrentBatchConfig into the renderer since it can now be local to a component which simplifies things a bit. Especially versioning but limits our ability to move it to become cross-renderer in the future.

@ConAntonakos
Copy link

ConAntonakos commented Oct 17, 2019

Are these being planned as hook versions for something like Suspense and lazy or am I way off?

@gaearon
Copy link
Collaborator

gaearon commented Oct 17, 2019

No (these are different things)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants