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

Consistent useCallback implementation in ReactDOMServer #18783

Merged
merged 1 commit into from
Apr 29, 2020

Conversation

alexmckenley
Copy link
Contributor

Summary

In ReactDOMServer we implement both useMemo and useCallback. The React docs mention that useCallback(fn, deps) is equivalent to useMemo(() => fn, deps), however this is not true today as useCallback completely ignores the deps argument.

Although the docs also mention that there are not semantic guarantees around memoization for useMemo (and subsequently useCallback), having the implementation differ between these hooks in the server environment can be confusing and lead to hard to track down issues that appear only during server rendering.

Also after reading the docs, I'm not absolutely sure if useCallback is expected to provide semantic guarantees based on the inputs changing. This is very clear for useMemo, but the behavior for useCallback seems somewhat up for debate, since its not explicitly called out.

I think it would be worth updating the docs to make this clear since I've already seen examples of devs relying on callback references to be stable (which is how this came to my attention initially).

Test Plan

Added test case in ReactDOMServerIntegrationHooks-test.js
Also verified that this prevents an infinite render loop on the server when used with code that relies on callback references being stable.

@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 ab9ee0f:

Sandbox Source
flamboyant-thunder-4684h Configuration

Copy link
Collaborator

@sebmarkbage sebmarkbage left a comment

Choose a reason for hiding this comment

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

Fine for consistency but I think the implementation of useMemo is actually wrong here. We should just break it and never memoize. This is not reliable anyway since suspense will clear it.

There are no guarantees that it'll memoize. If anything we should make it less predictable on the client to ensure that these abuses don't land in the first place. E.g. occasionally drop it in DEV.

It's not worth memoizing everything in the whole page just in case something calls setState in render during initial render, which should be a warning anyway since it only exists for update purposes. So it should never happen on the server initial render. In fact, we should break the whole implementation of setState during initial render on the server and free up that extra work.

I think I know which internal callsite you're referring to but that's poorly structured code relying on setState in render. We should restructure/rewrite that code.

@sizebot
Copy link

sizebot commented Apr 29, 2020

Details of bundled changes.

Comparing: 5153267...ab9ee0f

react-cache

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-cache.development.js 0.0% -0.1% 9.35 KB 9.35 KB 3.04 KB 3.03 KB UMD_DEV
react-cache.production.min.js 0.0% -0.4% 2.36 KB 2.36 KB 1.2 KB 1.2 KB UMD_PROD
react-cache.development.js 0.0% -0.1% 8.61 KB 8.61 KB 2.93 KB 2.92 KB NODE_DEV
react-cache.production.min.js 0.0% -0.4% 2.15 KB 2.15 KB 1.1 KB 1.1 KB NODE_PROD
ReactCache-dev.js +706.6% +391.1% 1.01 KB 8.16 KB 553 B 2.65 KB FB_WWW_DEV
ReactCache-prod.js 🔺+369.4% 🔺+165.0% 1.08 KB 5.09 KB 608 B 1.57 KB FB_WWW_PROD

Size changes (experimental)

Generated by 🚫 dangerJS against ab9ee0f

@sizebot
Copy link

sizebot commented Apr 29, 2020

Details of bundled changes.

Comparing: 5153267...ab9ee0f

react

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react.development.js 0.0% -0.0% 101.94 KB 101.94 KB 24.81 KB 24.81 KB UMD_DEV
react-unstable-cache.production.min.js 0.0% -0.5% 855 B 855 B 557 B 554 B NODE_PROD
react.production.min.js 0.0% -0.0% 11.39 KB 11.39 KB 4.55 KB 4.55 KB UMD_PROD
react-unstable-cache.profiling.min.js 0.0% -0.7% 854 B 854 B 557 B 553 B NODE_PROFILING
react-jsx-runtime.development.js 0.0% -0.0% 30.33 KB 30.33 KB 8.97 KB 8.97 KB NODE_DEV
react-jsx-runtime.production.min.js 0.0% -0.5% 947 B 947 B 596 B 593 B NODE_PROD
react-jsx-runtime.profiling.min.js 0.0% -0.7% 946 B 946 B 596 B 592 B NODE_PROFILING
react-unstable-cache.development.js 0.0% -1.0% 1.04 KB 1.04 KB 583 B 577 B NODE_DEV
react.profiling.min.js 0.0% -0.0% 14.9 KB 14.9 KB 5.63 KB 5.63 KB UMD_PROFILING
ReactCache-dev.js -87.6% -79.6% 8.16 KB 1.01 KB 2.65 KB 553 B FB_WWW_DEV
react.development.js 0.0% -0.0% 63.5 KB 63.5 KB 17.13 KB 17.13 KB NODE_DEV
ReactCache-prod.js -78.7% -62.3% 5.09 KB 1.08 KB 1.57 KB 608 B FB_WWW_PROD
react.production.min.js 0.0% -0.1% 6.32 KB 6.32 KB 2.62 KB 2.62 KB NODE_PROD
React-dev.js 0.0% -0.0% 98.87 KB 98.87 KB 24.18 KB 24.18 KB FB_WWW_DEV
React-prod.js 0.0% -0.1% 17.4 KB 17.4 KB 4.55 KB 4.55 KB FB_WWW_PROD
react-jsx-dev-runtime.development.js 0.0% -0.0% 29.75 KB 29.75 KB 8.8 KB 8.79 KB NODE_DEV
React-profiling.js 0.0% -0.1% 17.4 KB 17.4 KB 4.55 KB 4.55 KB FB_WWW_PROFILING
react-jsx-dev-runtime.production.min.js 0.0% -0.9% 441 B 441 B 316 B 313 B NODE_PROD
react-jsx-dev-runtime.profiling.min.js 0.0% -1.0% 440 B 440 B 314 B 311 B NODE_PROFILING

React: size: 0.0%, gzip: -0.0%

Size changes (stable)

Generated by 🚫 dangerJS against ab9ee0f

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.

4 participants