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

Fixed incompatibility between react-debug-tools and useContext() #14940

Merged
merged 5 commits into from
Feb 26, 2019

Conversation

bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Feb 24, 2019

An issue was reported in facebookarchive/redux-react-hook/issues/34 where DevTools caused a runtime error when inspecting a component that used a useContext hook. I determined the cause to be due to the fact that the useContext hook inside of ReactDebugHooks doesn't call nextHook() to advance the list, which causes subsequent hooks to be mismatched.

Initially, I thought the fix would be to simply add that call– but React itself is inconsistent with how it treats useContext between development and production builds. This presents a problem for the react-debug-tools package– it either breaks in development or production mode.

I've addressed this by refactoring our hooks ordering checks to use a separate, DEV only list (stored on fibers as _debugHookTypes). This way we don't have to rely on a hook being added to the hooks list in order to be validated, and code like ReactDebugHooks does not have to worry about inconsistent behavior between DEV and PROD bundles, (and we avoid adding additional overhead to PROD bundles).

This changed caught a couple of additional warnings in existing tests. I've beefed out or test coverage in this area as well for going forward.

A few alternative fixes were considered:

  1. Update the useContext implementation in React to use mountContext/updateContext (instead of calling readContext directly) so that it's consistent between modes.
    • Cons: This adds some small additional overhead in production mode that is otherwise unnecessary (and may not be beneficial to the majority of users). This also does not fix DevTools for the existing 16.8 releases.
  2. DevTools disables hooks inspection in production mode.
    • Cons: This makes DevTools slightly less useful, but that's already the case in production mode due to minification/mangling.
    • Pros: This is a backwards compatible fix (meaning react-debug-tools will work with earlier hook releases too).
  3. DevTools passes the buildType flag to inspectHooks (telling it whether React is running in production mode). ReactDebugHooks.useContext then conditional calls nextHook() based on this flag.
    • Cons: This feels a little fragile and the API feels a little awkward (since there's already a third optional param).
    • Pros: This is a backwards compatible fix.

@bdbch
Copy link

bdbch commented Feb 24, 2019

Great! Thanks a lot!

@3dx-tech
Copy link

Well done! Thx!

@@ -93,6 +93,7 @@ function useContext<T>(
context: ReactContext<T>,
observedBits: void | number | boolean,
): T {
nextHook();
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if relevant, but we add the workinprogresshook to the list only when DEV. so hook.next behaves differently in prod vs dev https://github.com/facebook/react/blob/master/packages/react-reconciler/src/ReactFiberHooks.js#L527-L529

Copy link
Contributor Author

@bvaughn bvaughn Feb 24, 2019

Choose a reason for hiding this comment

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

Good catch. My test had a blindspot here since the null hook would just re-initialize the state. I'll change it to throw if that happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... 🤔 I think this might be trickier than a simple __DEV__ check can account for. It's not a matter of whether react-debug-tools is itself in production or development mode, but whether the renderer it's interacting with is.

@bvaughn bvaughn force-pushed the fix-react-debug-tools-useContext branch 2 times, most recently from d8b9115 to 73a1cae Compare February 24, 2019 16:25
bvaughn pushed a commit to bvaughn/react-devtools-experimental that referenced this pull request Feb 24, 2019
@bvaughn bvaughn force-pushed the fix-react-debug-tools-useContext branch from 73a1cae to e777462 Compare February 24, 2019 16:37
@threepointone
Copy link
Contributor

Maybe we can change it to add the hook to the list whether dev or prod. I remember adding it only so we could do the warning for when the order doesn’t match across renders. It doesn’t serve any other purpose.

@bvaughn
Copy link
Contributor Author

bvaughn commented Feb 24, 2019

Yeah, I'm looking into that now.

@bvaughn
Copy link
Contributor Author

bvaughn commented Feb 24, 2019

Previously in production mode, useContext pointed directly at readContext. Commit 3f5cde5 changes it to point to mountContext/updateContext instead (and changes those to both call mountWorkInProgressHook/updateWorkInProgressHook unconditionally). This adds additional overhead in production mode that would be nice to avoid, but without it– the react-debug-tools package can't properly inspect components– since it doesn't know whether to advance the hook index for context.

If this change is unacceptable, then we have a few alternatives:

  1. DevTools disables hooks inspection in production mode. (This would be a bit unfortunate but not the end of the world.)
  2. DevTools passes the buildType flag to inspectHooks (telling it whether React is running in production mode). ReactDebugHooks.useContext then conditional calls nextHook() based on this flag. (This seems a bit fragile.)

The above two alternatives have a couple of benefits:

  • They would adding overhead to React in production mode for a feature that might not be used by many people.
  • They would enable DevTools to work properly with existing hooks-compatible React versions.

I would be interested in hearing @sebmarkbage's thoughts on this.

@gaearon
Copy link
Collaborator

gaearon commented Feb 25, 2019

I'd err on the side of keeping context calls very cheap and accepting some integration fragility (DevTools pass the DEV flag) as a tradeoff. With a test that shouldn't even be that fragile.

@sebmarkbage
Copy link
Collaborator

My M.O. in any case like this is to error on the side of adding more implementation complexity to the DEV side and keep the prod side clean.

In this case, let’s take a step back and ask why DEV differs.

It’s easy to assume that context is the odd one out here and that all other hooks goes into the hooks list. That’s not true conceptually though.

Some of the hooks we have now don’t have to be stateful (eg useEffect without a dep list nor destructor). In the future, the hooks we want to add will also not be stateful.

So the list we have right now is not so much the canonical “hooks list”. It’s only the “stateful hooks list”. In fact, in the future some of them might have more than one entry.

I don’t know exactly but I’m guessing that the only reason @acdlite added context to this set in DEV is that it makes it easier to issue warnings that enforce call order and preserve our option to keep them. However, by reusing the production mechanism for this, I think we’ve also accidentally introduced a bug (prod/dev behavior) that is observable through the invariant calls that only happen in one path.

The lesson learned from “current owner” is that it’s a bad idea to rely on subtle production behavior for dev warnings because they’re not always going to overlap. That’s why we have “current debug frame” instead for the warning info.

I think it would be appropriate to do the same here if we want to preserve some of the dev warnings. If we want to conceptually model the warnings as the user facing api contract (every primitive hook call is one hook in that exact order). Then we should explicitly model that in dev. Which requires dev to have a separate hooks list that overlaps mostly with the stateful hooks list (but not exactly).

@bvaughn
Copy link
Contributor Author

bvaughn commented Feb 25, 2019

I think it would be appropriate to do the same here if we want to preserve some of the dev warnings. If we want to conceptually model the warnings as the user facing api contract (every primitive hook call is one hook in that exact order). Then we should explicitly model that in dev. Which requires dev to have a separate hooks list that overlaps mostly with the stateful hooks list (but not exactly).

Interesting. I hadn't considered this option, but I dig it.

@bvaughn
Copy link
Contributor Author

bvaughn commented Feb 25, 2019

Looks like the DEV warning is already broken for most hook types other than useState and useReducer. Either an invariant or a runtime error will be triggered before our DEV warning. Edit This was just an artifact of the way our tests were written.

I'll expand our hook ordering test to make sure it covers all of the hook types. There are definitely cases where we'll warn about other things as well when the order changes (e.g. inputs array changing) but at least we can ensure that we're warning about the order changing.

@bvaughn
Copy link
Contributor Author

bvaughn commented Feb 25, 2019

Okay. I've refactored this to a DEV-only list of hook names to verify ordering. I've also updated our tests to cover more cases (including several that failed prior to this change).

There are a couple of potential follow up things we could do (which I left TODO comments for).

@bvaughn bvaughn requested a review from acdlite February 25, 2019 23:01
@sizebot
Copy link

sizebot commented Feb 25, 2019

ReactDOM: size: 0.0%, gzip: 0.0%

Details of bundled changes.

Comparing: 0b8efb2...3899681

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js +0.7% +0.4% 767.62 KB 773.15 KB 175.11 KB 175.77 KB UMD_DEV
react-dom.production.min.js 0.0% 0.0% 105.31 KB 105.31 KB 33.91 KB 33.91 KB UMD_PROD
react-dom.profiling.min.js 0.0% 0.0% 108.22 KB 108.22 KB 34.8 KB 34.8 KB UMD_PROFILING
react-dom.development.js +0.7% +0.4% 762.25 KB 767.79 KB 173.6 KB 174.26 KB NODE_DEV
react-dom.production.min.js 0.0% 0.0% 105.52 KB 105.52 KB 33.41 KB 33.41 KB NODE_PROD
react-dom.profiling.min.js 0.0% 0.0% 108.6 KB 108.6 KB 34.22 KB 34.22 KB NODE_PROFILING
ReactDOM-dev.js +0.7% +0.4% 785.25 KB 790.78 KB 174.77 KB 175.43 KB FB_WWW_DEV
ReactDOM-prod.js 0.0% 0.0% 322.24 KB 322.24 KB 58.72 KB 58.72 KB FB_WWW_PROD
react-dom-unstable-fire.development.js +0.7% +0.4% 767.96 KB 773.5 KB 175.24 KB 175.91 KB UMD_DEV
react-dom-unstable-fire.production.min.js 0.0% 0.0% 105.32 KB 105.32 KB 33.92 KB 33.92 KB UMD_PROD
react-dom-unstable-fire.profiling.min.js 0.0% 0.0% 108.24 KB 108.24 KB 34.8 KB 34.8 KB UMD_PROFILING
react-dom-unstable-fire.development.js +0.7% +0.4% 762.6 KB 768.13 KB 173.74 KB 174.4 KB NODE_DEV
react-dom-unstable-fire.production.min.js 0.0% 0.0% 105.54 KB 105.54 KB 33.42 KB 33.42 KB NODE_PROD
react-dom-unstable-fire.profiling.min.js 0.0% 0.0% 108.61 KB 108.61 KB 34.23 KB 34.23 KB NODE_PROFILING
ReactFire-dev.js +0.7% +0.4% 784.46 KB 789.99 KB 174.73 KB 175.38 KB FB_WWW_DEV
react-dom-test-utils.development.js 0.0% 0.0% 47.06 KB 47.06 KB 12.99 KB 12.99 KB UMD_DEV
react-dom-test-utils.production.min.js 0.0% 0.0% 10.27 KB 10.27 KB 3.8 KB 3.8 KB UMD_PROD
react-dom-test-utils.development.js 0.0% 0.0% 46.78 KB 46.78 KB 12.92 KB 12.92 KB NODE_DEV
react-dom-test-utils.production.min.js 0.0% 🔺+0.1% 10.05 KB 10.05 KB 3.73 KB 3.73 KB NODE_PROD
react-dom-unstable-native-dependencies.development.js 0.0% 0.0% 60.61 KB 60.61 KB 15.92 KB 15.92 KB UMD_DEV
react-dom-unstable-native-dependencies.production.min.js 0.0% 🔺+0.1% 11.01 KB 11.01 KB 3.81 KB 3.81 KB UMD_PROD
react-dom-unstable-native-dependencies.development.js 0.0% 0.0% 60.28 KB 60.28 KB 15.79 KB 15.79 KB NODE_DEV
react-dom-unstable-native-dependencies.production.min.js 0.0% 🔺+0.1% 10.75 KB 10.75 KB 3.71 KB 3.71 KB NODE_PROD
react-dom-server.browser.development.js 0.0% 0.0% 127.39 KB 127.39 KB 33.95 KB 33.95 KB UMD_DEV
react-dom-server.browser.production.min.js 0.0% 0.0% 18.89 KB 18.89 KB 7.22 KB 7.22 KB UMD_PROD
react-dom-server.browser.development.js 0.0% 0.0% 123.52 KB 123.52 KB 33.02 KB 33.02 KB NODE_DEV
react-dom-server.browser.production.min.js 0.0% 0.0% 18.81 KB 18.81 KB 7.21 KB 7.21 KB NODE_PROD
ReactDOMServer-dev.js 0.0% 0.0% 124.37 KB 124.37 KB 32.47 KB 32.47 KB FB_WWW_DEV
react-dom-server.node.development.js 0.0% 0.0% 125.58 KB 125.58 KB 33.56 KB 33.56 KB NODE_DEV
react-dom-server.node.production.min.js 0.0% 0.0% 19.69 KB 19.69 KB 7.52 KB 7.52 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.production.min.js 0.0% 🔺+0.1% 1.21 KB 1.21 KB 706 B 707 B UMD_PROD
react-dom-unstable-fizz.browser.development.js 0.0% +0.2% 3.45 KB 3.45 KB 1.39 KB 1.39 KB NODE_DEV
react-dom-unstable-fizz.browser.production.min.js 0.0% 🔺+0.2% 1.05 KB 1.05 KB 637 B 638 B NODE_PROD
react-dom-unstable-fizz.node.development.js 0.0% +0.2% 3.7 KB 3.7 KB 1.42 KB 1.42 KB NODE_DEV
react-dom-unstable-fizz.node.production.min.js 0.0% 🔺+0.3% 1.1 KB 1.1 KB 666 B 668 B NODE_PROD

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.development.js +1.0% +0.6% 543.7 KB 549.23 KB 118.34 KB 119.02 KB UMD_DEV
react-art.production.min.js 0.0% 0.0% 97.31 KB 97.31 KB 29.85 KB 29.85 KB UMD_PROD
react-art.development.js +1.2% +0.7% 474.78 KB 480.32 KB 101.09 KB 101.75 KB NODE_DEV
react-art.production.min.js 0.0% 0.0% 62.4 KB 62.4 KB 19.01 KB 19.02 KB NODE_PROD
ReactART-dev.js +1.1% +0.7% 484.04 KB 489.56 KB 100.32 KB 100.98 KB FB_WWW_DEV
ReactART-prod.js 0.0% 0.0% 195.46 KB 195.46 KB 33.04 KB 33.04 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.9% +0.5% 610.95 KB 616.48 KB 131.04 KB 131.7 KB RN_FB_DEV
ReactNativeRenderer-prod.js 0.0% 0.0% 246.83 KB 246.83 KB 43.07 KB 43.07 KB RN_FB_PROD
ReactNativeRenderer-profiling.js 0.0% 0.0% 253.18 KB 253.18 KB 44.62 KB 44.62 KB RN_FB_PROFILING
ReactNativeRenderer-dev.js +0.9% +0.5% 610.87 KB 616.39 KB 131 KB 131.67 KB RN_OSS_DEV
ReactNativeRenderer-prod.js 0.0% 0.0% 246.84 KB 246.84 KB 43.06 KB 43.06 KB RN_OSS_PROD
ReactNativeRenderer-profiling.js 0.0% 0.0% 253.19 KB 253.19 KB 44.61 KB 44.61 KB RN_OSS_PROFILING
ReactFabric-dev.js +0.9% +0.5% 601.81 KB 607.33 KB 128.75 KB 129.42 KB RN_FB_DEV
ReactFabric-prod.js 0.0% 0.0% 239.17 KB 239.17 KB 41.59 KB 41.59 KB RN_FB_PROD
ReactFabric-profiling.js 0.0% 0.0% 245.4 KB 245.4 KB 43.12 KB 43.12 KB RN_FB_PROFILING
ReactFabric-dev.js +0.9% +0.5% 601.71 KB 607.24 KB 128.71 KB 129.37 KB RN_OSS_DEV
ReactFabric-prod.js 0.0% 0.0% 239.18 KB 239.18 KB 41.58 KB 41.58 KB RN_OSS_PROD
ReactFabric-profiling.js 0.0% 0.0% 245.41 KB 245.41 KB 43.12 KB 43.12 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 +1.1% +0.6% 486.63 KB 492.17 KB 103.42 KB 104.08 KB UMD_DEV
react-test-renderer.production.min.js 0.0% 0.0% 63.72 KB 63.72 KB 19.52 KB 19.52 KB UMD_PROD
react-test-renderer.development.js +1.2% +0.6% 481.07 KB 486.61 KB 102.11 KB 102.77 KB NODE_DEV
react-test-renderer.production.min.js 0.0% 0.0% 63.38 KB 63.38 KB 19.18 KB 19.18 KB NODE_PROD
ReactTestRenderer-dev.js +1.1% +0.7% 491.15 KB 496.67 KB 101.72 KB 102.38 KB FB_WWW_DEV
react-test-renderer-shallow.development.js 0.0% 0.0% 37.2 KB 37.2 KB 9.5 KB 9.5 KB UMD_DEV
react-test-renderer-shallow.production.min.js 0.0% 🔺+0.1% 11.12 KB 11.12 KB 3.35 KB 3.35 KB UMD_PROD
react-test-renderer-shallow.development.js 0.0% 0.0% 31.5 KB 31.5 KB 8.14 KB 8.14 KB NODE_DEV
react-test-renderer-shallow.production.min.js 0.0% 🔺+0.1% 11.77 KB 11.77 KB 3.65 KB 3.66 KB NODE_PROD

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler.development.js +1.2% +0.7% 472.13 KB 477.67 KB 99.45 KB 100.11 KB NODE_DEV
react-reconciler.production.min.js 0.0% 0.0% 63.56 KB 63.56 KB 18.79 KB 18.79 KB NODE_PROD
react-reconciler-persistent.development.js +1.2% +0.7% 470.33 KB 475.86 KB 98.74 KB 99.4 KB NODE_DEV
react-reconciler-persistent.production.min.js 0.0% 0.0% 63.57 KB 63.57 KB 18.79 KB 18.8 KB NODE_PROD
react-reconciler-reflection.development.js 0.0% 0.0% 15.76 KB 15.76 KB 4.98 KB 4.98 KB NODE_DEV
react-reconciler-reflection.production.min.js 0.0% 🔺+0.1% 2.7 KB 2.7 KB 1.22 KB 1.23 KB NODE_PROD

Generated by 🚫 dangerJS

@bvaughn bvaughn changed the title Added missing nextHook() call to react-debug-tools useContext() impl Fixed incompatibility between react-debug-tools and useContext() Feb 25, 2019
Brian Vaughn added 4 commits February 26, 2019 10:25
This adds a small amount of overhead, so I'm not sure if it will fly. I think it might be necessary though in order to support the react-debug-tools package.
This enables us to warn about more cases (e.g. useContext, useDebugValue) withou the need to add any overhead to production bundles.
@bvaughn bvaughn force-pushed the fix-react-debug-tools-useContext branch from ef8bd2c to c2fc659 Compare February 26, 2019 18:54
@bvaughn
Copy link
Contributor Author

bvaughn commented Feb 26, 2019

Rebased and updated DEV mode to also check for mount vs update using the nextCurrentHook.

'1. useReducer useReducer\n' +
'2. useState useRef\n' +
' ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n\n',
]);
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 wasn't strictly necessary but it seems like this test is a bit more robust (and reads better) if it uses our toWarnDev check.

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