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

Disallow reading context during useMemo etc #14653

Merged
merged 6 commits into from
Jan 23, 2019

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Jan 21, 2019

Reverts #14651

In case we want to get it in

@gaearon gaearon force-pushed the revert-14651-revert-14648-ctx-bad branch from 700680a to 7cb26fb Compare January 22, 2019 14:52
@sizebot
Copy link

sizebot commented Jan 22, 2019

ReactDOM: size: -0.1%, gzip: -0.1%

Details of bundled changes.

Comparing: 5fce648...223960e

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js +0.2% +0.1% 734.92 KB 736.19 KB 169.37 KB 169.6 KB UMD_DEV
react-dom.production.min.js -0.1% -0.1% 98.98 KB 98.88 KB 32.2 KB 32.18 KB UMD_PROD
react-dom.profiling.min.js -0.1% -0.0% 101.97 KB 101.86 KB 32.84 KB 32.83 KB UMD_PROFILING
react-dom.development.js +0.2% +0.1% 729.98 KB 731.25 KB 167.94 KB 168.17 KB NODE_DEV
react-dom.production.min.js -0.1% -0.0% 98.99 KB 98.88 KB 31.68 KB 31.67 KB NODE_PROD
react-dom.profiling.min.js -0.1% -0.1% 102.09 KB 101.98 KB 32.31 KB 32.29 KB NODE_PROFILING
ReactDOM-dev.js +0.2% +0.1% 752.51 KB 753.78 KB 169.28 KB 169.48 KB FB_WWW_DEV
ReactDOM-prod.js 🔺+0.3% 🔺+0.1% 311.52 KB 312.5 KB 57.5 KB 57.56 KB FB_WWW_PROD
ReactDOM-profiling.js +0.3% +0.1% 318.68 KB 319.66 KB 58.83 KB 58.88 KB FB_WWW_PROFILING
react-dom-unstable-fire.development.js +0.2% +0.1% 735.27 KB 736.54 KB 169.51 KB 169.74 KB UMD_DEV
react-dom-unstable-fire.production.min.js -0.1% -0.1% 99 KB 98.89 KB 32.21 KB 32.18 KB UMD_PROD
react-dom-unstable-fire.profiling.min.js -0.1% -0.0% 101.98 KB 101.88 KB 32.85 KB 32.83 KB UMD_PROFILING
react-dom-unstable-fire.development.js +0.2% +0.1% 730.33 KB 731.6 KB 168.09 KB 168.31 KB NODE_DEV
react-dom-unstable-fire.production.min.js -0.1% -0.0% 99 KB 98.9 KB 31.69 KB 31.68 KB NODE_PROD
react-dom-unstable-fire.profiling.min.js -0.1% -0.1% 102.1 KB 102 KB 32.32 KB 32.3 KB NODE_PROFILING
ReactFire-dev.js +0.2% +0.1% 751.72 KB 752.99 KB 169.23 KB 169.42 KB FB_WWW_DEV
ReactFire-prod.js 🔺+0.3% 🔺+0.1% 300.11 KB 301.09 KB 55.23 KB 55.26 KB FB_WWW_PROD
ReactFire-profiling.js +0.3% +0.1% 307.2 KB 308.18 KB 56.51 KB 56.55 KB FB_WWW_PROFILING
react-dom-test-utils.development.js 0.0% -0.0% 44.87 KB 44.87 KB 12.3 KB 12.3 KB UMD_DEV
react-dom-unstable-native-dependencies.development.js 0.0% -0.0% 60.29 KB 60.29 KB 15.79 KB 15.79 KB NODE_DEV
react-dom-unstable-native-dependencies.production.min.js 0.0% -0.0% 10.75 KB 10.75 KB 3.71 KB 3.71 KB NODE_PROD
react-dom-server.browser.development.js 0.0% -0.0% 125.09 KB 125.09 KB 33.35 KB 33.35 KB UMD_DEV
react-dom-server.browser.production.min.js 0.0% -0.0% 16.87 KB 16.87 KB 6.51 KB 6.51 KB NODE_PROD
react-dom-unstable-fizz.browser.development.js 0.0% -0.1% 3.63 KB 3.63 KB 1.44 KB 1.44 KB UMD_DEV
react-dom-unstable-fizz.browser.development.js 0.0% -0.1% 3.45 KB 3.45 KB 1.39 KB 1.39 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.2% +0.2% 513.77 KB 515.04 KB 113.21 KB 113.44 KB UMD_DEV
react-art.production.min.js -0.1% -0.0% 91.04 KB 90.93 KB 27.97 KB 27.96 KB UMD_PROD
react-art.development.js +0.3% +0.2% 445.29 KB 446.56 KB 96.09 KB 96.32 KB NODE_DEV
react-art.production.min.js -0.2% -0.1% 56.01 KB 55.91 KB 17.25 KB 17.22 KB NODE_PROD
ReactART-dev.js +0.3% +0.2% 454.23 KB 455.5 KB 95.32 KB 95.52 KB FB_WWW_DEV
ReactART-prod.js 🔺+0.5% 🔺+0.3% 187.42 KB 188.4 KB 32.15 KB 32.23 KB FB_WWW_PROD

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-dev.js +0.2% +0.2% 578.7 KB 579.97 KB 125.83 KB 126.03 KB RN_FB_DEV
ReactNativeRenderer-prod.js 🔺+0.4% 🔺+0.2% 243.02 KB 244 KB 42.77 KB 42.84 KB RN_FB_PROD
ReactNativeRenderer-profiling.js +0.4% +0.1% 249.17 KB 250.15 KB 44.12 KB 44.17 KB RN_FB_PROFILING
ReactNativeRenderer-dev.js +0.2% +0.2% 578.61 KB 579.88 KB 125.79 KB 125.99 KB RN_OSS_DEV
ReactNativeRenderer-prod.js -0.1% -0.2% 227.3 KB 227.04 KB 39.53 KB 39.46 KB RN_OSS_PROD
ReactNativeRenderer-profiling.js -0.1% -0.2% 233.31 KB 233.05 KB 40.91 KB 40.85 KB RN_OSS_PROFILING
ReactFabric-dev.js +0.2% +0.2% 569.55 KB 570.82 KB 123.54 KB 123.75 KB RN_FB_DEV
ReactFabric-prod.js 🔺+0.4% 🔺+0.2% 236.22 KB 237.2 KB 41.29 KB 41.38 KB RN_FB_PROD
ReactFabric-profiling.js +0.4% +0.1% 241.52 KB 242.5 KB 42.65 KB 42.7 KB RN_FB_PROFILING
ReactFabric-dev.js +0.2% +0.2% 569.46 KB 570.73 KB 123.5 KB 123.71 KB RN_OSS_DEV
ReactFabric-prod.js -0.1% -0.2% 220.53 KB 220.27 KB 38.07 KB 38 KB RN_OSS_PROD
ReactFabric-profiling.js -0.1% -0.2% 225.62 KB 225.36 KB 39.45 KB 39.39 KB RN_OSS_PROFILING

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer.development.js +0.3% +0.2% 458.5 KB 459.78 KB 98.85 KB 99.08 KB UMD_DEV
react-test-renderer.production.min.js -0.2% -0.1% 57.41 KB 57.31 KB 17.63 KB 17.61 KB UMD_PROD
react-test-renderer.development.js +0.3% +0.2% 453.46 KB 454.73 KB 97.62 KB 97.85 KB NODE_DEV
react-test-renderer.production.min.js -0.2% -0.1% 57.08 KB 56.98 KB 17.47 KB 17.45 KB NODE_PROD
ReactTestRenderer-dev.js +0.3% +0.2% 462.62 KB 463.89 KB 97.2 KB 97.4 KB FB_WWW_DEV
react-test-renderer-shallow.development.js 0.0% -0.0% 38.09 KB 38.09 KB 9.83 KB 9.83 KB UMD_DEV

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler.development.js +0.3% +0.2% 443.12 KB 444.39 KB 94.59 KB 94.83 KB NODE_DEV
react-reconciler.production.min.js -0.2% -0.0% 57.17 KB 57.07 KB 17.07 KB 17.07 KB NODE_PROD
react-reconciler-persistent.development.js +0.3% +0.2% 441.5 KB 442.77 KB 93.95 KB 94.18 KB NODE_DEV
react-reconciler-persistent.production.min.js -0.2% -0.0% 57.18 KB 57.08 KB 17.08 KB 17.07 KB NODE_PROD
react-reconciler-reflection.development.js 0.0% 0.0% 15.4 KB 15.4 KB 4.83 KB 4.83 KB NODE_DEV

Generated by 🚫 dangerJS

@gaearon gaearon changed the title Resubmit "Disallow reading context during useMemo etc" Disallow reading context during useMemo etc Jan 22, 2019
@gaearon
Copy link
Collaborator Author

gaearon commented Jan 22, 2019

I pushed another commit that introduces stashing to NewContext so that we don't need to rely on expirationTime.

@acdlite Please let me know if this is what you meant — and whether the edge case test I added in 093c8a2 and 7cb26fb is sufficient. (I couldn't get it to fail with the old approach so I probably still misunderstand the issue.)

@gaearon gaearon requested a review from acdlite January 22, 2019 15:10
@gaearon gaearon force-pushed the revert-14651-revert-14648-ctx-bad branch from f0a8b44 to cfbdf90 Compare January 22, 2019 15:13
@acdlite
Copy link
Collaborator

acdlite commented Jan 22, 2019

This looks good to me, gimme a few moments to try to break it and then I'll stamp

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.

Ok looks good!

I think we need a guard here, too:

const eagerState = eagerReducer(currentState, action);

It's definitely an edge case. I think it would only happen if you called setState during the render phase of a class component, and the class component was in a different context tree than the component you're updating.

@acdlite
Copy link
Collaborator

acdlite commented Jan 22, 2019

Also that reminds me we need a follow-up at some point to add the same guard to class component render phase functions. (Not super urgent, IMO, since that's already in master.)

@gaearon
Copy link
Collaborator Author

gaearon commented Jan 22, 2019

The same guard being what exactly? I thought we already can’t call Hooks from classes.

@acdlite
Copy link
Collaborator

acdlite commented Jan 22, 2019

You can call readContext though

@gaearon
Copy link
Collaborator Author

gaearon commented Jan 22, 2019

Right but when (during class render) would we want to disable it? It’s already disabled outside of class render.

@gaearon
Copy link
Collaborator Author

gaearon commented Jan 22, 2019

In other words — I thought the whole point of readContext is it should work in classes.

@acdlite
Copy link
Collaborator

acdlite commented Jan 22, 2019

Not in a setState reducer though

@gaearon
Copy link
Collaborator Author

gaearon commented Jan 23, 2019

Aaah.

@gaearon
Copy link
Collaborator Author

gaearon commented Jan 23, 2019

What makes setState reducer conceptually different from gDSFP? The message says we support in in render phase lifecycles including render and gDSFP.

@gaearon gaearon force-pushed the revert-14651-revert-14648-ctx-bad branch from 005e057 to 223960e Compare January 23, 2019 15:51
@gaearon gaearon merged commit a129259 into master Jan 23, 2019
@gaearon gaearon deleted the revert-14651-revert-14648-ctx-bad branch January 23, 2019 15:52
@acdlite
Copy link
Collaborator

acdlite commented Jan 23, 2019

Left comment on the PR: #14670 (comment)

@sebmarkbage
Copy link
Collaborator

I think even this is too much for prod.

Hooks are about twice as slow as they should be already and this is adding to a bunch of micro optimizing we need to do.

In a better typed form we can guarantee that this never happens. The best way we have to ensure that right now is warnings.

As long as we can warn in all cases someone does this, I think it’s fine to leave it in production code since at worst you’re getting what you expected anyway.

@gaearon
Copy link
Collaborator Author

gaearon commented Jan 23, 2019

Should I... revert? :-)

@sebmarkbage
Copy link
Collaborator

Well you could land this and follow up by making all the stash things set a flag that triggers a warning in dev. :)

@gaearon
Copy link
Collaborator Author

gaearon commented Jan 23, 2019

What about "can't call Hooks from inside useMemo", wanna make it DEV only too?

@sebmarkbage
Copy link
Collaborator

I’m most concerned about useMemo and the initial state/action calls in useState/useReducer since those happens just automatically during render. They’re not trigger by an update. So they scale up by the size of the render tree.

I haven’t vetted all the reducer callsites if they only happen if there is an actual update in the queue or if they always happen.

But for consistency we might as well make them all DEV only.

gaearon added a commit to gaearon/react that referenced this pull request Jan 23, 2019
…throws

I introduced this in facebook#14653 (223960e). But I relied on restart to clean it up in case of error, and didn't notice the try/catch. This makes sure the failing case also restores the current variable which renderWitHooks() currently relies on.
@gaearon
Copy link
Collaborator Author

gaearon commented Jan 23, 2019

Making it all DEV-only in #14677

n8schloss pushed a commit to n8schloss/react that referenced this pull request Jan 31, 2019
* Revert "Revert "Double-render function components with Hooks in DEV in StrictMode" (facebook#14652)"

This reverts commit 3fbebb2.

* Revert "Revert "Disallow reading context during useMemo etc" (facebook#14651)"

This reverts commit 5fce648.

* Add extra passing test for an edge case

Mentioned by @acdlite to watch out for

* More convoluted test

* Don't rely on expirationTime

Addresses @acdlite's concerns

* Edge case: forbid readContext() during eager reducer
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.

5 participants