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

RFC: warn when returning different hooks on subsequent renders #14585

Merged
merged 41 commits into from Jan 22, 2019

Conversation

Projects
None yet
9 participants
@threepointone
Copy link
Contributor

threepointone commented Jan 14, 2019

^ like it says. adds a field to Hook to track hook 'type', and compares when cloning on further renders.

started this PR/mvp to discuss how you folks would like this implemented?

I have run the tests (and added one!)

threepointone added some commits Jan 14, 2019

warn when returning different hooks on next render
like it says. adds a field to Hook to track effect 'type', and compares when cloning subsequently.
@acdlite

This comment has been minimized.

Copy link
Member

acdlite commented Jan 14, 2019

Maybe we could use react-debug-tools to print a warning that shows the expected the hook structure versus the actual one cc @bvaughn

@sizebot

This comment has been minimized.

Copy link

sizebot commented Jan 14, 2019

ReactDOM: size: 0.0%, gzip: 0.0%

Details of bundled changes.

Comparing: 3fbebb2...3ad6523

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js +0.5% +0.6% 733.92 KB 737.53 KB 169.29 KB 170.3 KB UMD_DEV
react-dom.production.min.js 0.0% 0.0% 98.98 KB 98.98 KB 32.2 KB 32.2 KB UMD_PROD
react-dom.profiling.min.js 0.0% 0.0% 101.97 KB 101.97 KB 32.83 KB 32.84 KB UMD_PROFILING
react-dom.development.js +0.5% +0.6% 728.98 KB 732.59 KB 167.87 KB 168.87 KB NODE_DEV
react-dom.production.min.js 0.0% 0.0% 98.99 KB 98.99 KB 31.68 KB 31.68 KB NODE_PROD
react-dom.profiling.min.js 0.0% 0.0% 102.09 KB 102.09 KB 32.3 KB 32.3 KB NODE_PROFILING
ReactDOM-dev.js +0.5% +0.6% 751.2 KB 755.16 KB 169.1 KB 170.08 KB FB_WWW_DEV
react-dom-unstable-fire.development.js +0.5% +0.6% 734.26 KB 737.88 KB 169.43 KB 170.44 KB UMD_DEV
react-dom-unstable-fire.production.min.js 0.0% 0.0% 99 KB 99 KB 32.2 KB 32.21 KB UMD_PROD
react-dom-unstable-fire.profiling.min.js 0.0% 0.0% 101.98 KB 101.98 KB 32.84 KB 32.85 KB UMD_PROFILING
react-dom-unstable-fire.development.js +0.5% +0.6% 729.32 KB 732.94 KB 168.01 KB 169.02 KB NODE_DEV
react-dom-unstable-fire.production.min.js 0.0% 0.0% 99 KB 99 KB 31.69 KB 31.69 KB NODE_PROD
react-dom-unstable-fire.profiling.min.js 0.0% 0.0% 102.1 KB 102.1 KB 32.31 KB 32.31 KB NODE_PROFILING
ReactFire-dev.js +0.5% +0.6% 750.41 KB 754.37 KB 169.04 KB 170.03 KB FB_WWW_DEV
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-test-utils.production.min.js 0.0% 🔺+0.1% 9.97 KB 9.97 KB 3.71 KB 3.71 KB UMD_PROD
react-dom-test-utils.development.js 0.0% 0.0% 44.59 KB 44.59 KB 12.23 KB 12.24 KB NODE_DEV
react-dom-test-utils.production.min.js 0.0% 0.0% 9.74 KB 9.74 KB 3.65 KB 3.65 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.29 KB 60.29 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.7 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.development.js 0.0% 0.0% 121.22 KB 121.22 KB 32.43 KB 32.43 KB NODE_DEV
react-dom-server.node.development.js 0.0% 0.0% 123.28 KB 123.28 KB 32.97 KB 32.97 KB NODE_DEV
react-dom-server.node.production.min.js 0.0% 0.0% 17.74 KB 17.74 KB 6.83 KB 6.83 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 704 B 705 B UMD_PROD
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.browser.production.min.js 0.0% 🔺+0.2% 1.05 KB 1.05 KB 635 B 636 B NODE_PROD
react-dom-unstable-fizz.node.development.js 0.0% +0.1% 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 665 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.7% +0.9% 512.77 KB 516.38 KB 113.13 KB 114.13 KB UMD_DEV
react-art.production.min.js 0.0% 0.0% 91.04 KB 91.04 KB 27.97 KB 27.97 KB UMD_PROD
react-art.development.js +0.8% +1.0% 444.29 KB 447.9 KB 96.02 KB 97.02 KB NODE_DEV
react-art.production.min.js 0.0% 0.0% 56.01 KB 56.01 KB 17.25 KB 17.25 KB NODE_PROD
ReactART-dev.js +0.9% +1.0% 452.92 KB 456.88 KB 95.14 KB 96.13 KB FB_WWW_DEV
ReactART-prod.js 0.0% 0.0% 187.42 KB 187.42 KB 32.15 KB 32.15 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.7% +0.8% 577.38 KB 581.35 KB 125.71 KB 126.68 KB RN_FB_DEV
ReactNativeRenderer-dev.js +0.7% +0.8% 577.3 KB 581.26 KB 125.67 KB 126.64 KB RN_OSS_DEV
ReactFabric-dev.js +0.7% +0.8% 568.24 KB 572.2 KB 123.42 KB 124.41 KB RN_FB_DEV
ReactFabric-dev.js +0.7% +0.8% 568.14 KB 572.11 KB 123.38 KB 124.37 KB RN_OSS_DEV
ReactFabric-profiling.js 0.0% 0.0% 225.62 KB 225.62 KB 39.45 KB 39.45 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.8% +1.0% 457.5 KB 461.12 KB 98.78 KB 99.79 KB UMD_DEV
react-test-renderer.production.min.js 0.0% 0.0% 57.41 KB 57.41 KB 17.62 KB 17.62 KB UMD_PROD
react-test-renderer.development.js +0.8% +1.0% 452.46 KB 456.08 KB 97.55 KB 98.56 KB NODE_DEV
react-test-renderer.production.min.js 0.0% 0.0% 57.08 KB 57.08 KB 17.47 KB 17.47 KB NODE_PROD
ReactTestRenderer-dev.js +0.9% +1.0% 461.3 KB 465.27 KB 97.02 KB 98 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-test-renderer-shallow.production.min.js 0.0% 0.0% 10.56 KB 10.56 KB 3.25 KB 3.25 KB UMD_PROD

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler.development.js +0.8% +1.1% 442.12 KB 445.73 KB 94.53 KB 95.53 KB NODE_DEV
react-reconciler.production.min.js 0.0% 0.0% 57.17 KB 57.17 KB 17.07 KB 17.07 KB NODE_PROD
react-reconciler-persistent.development.js +0.8% +1.1% 440.49 KB 444.11 KB 93.88 KB 94.88 KB NODE_DEV
react-reconciler-persistent.production.min.js 0.0% 0.0% 57.18 KB 57.18 KB 17.08 KB 17.08 KB NODE_PROD

Generated by 🚫 dangerJS

@acdlite

This comment has been minimized.

Copy link
Member

acdlite commented Jan 14, 2019

I’m thinking the message should emphasize the name of the top most hook that doesn’t match. Otherwise it’s not very helpful for custom hooks.

@threepointone threepointone changed the title RFC: warn when returning different hooks on subsequent render RFC: warn when returning different hooks on subsequent renders Jan 14, 2019

@threepointone

This comment has been minimized.

Copy link
Contributor Author

threepointone commented Jan 14, 2019

I have an odd bug in my test, where even though I'm matching the warning message exactly, toWarnDev isn't matching it right. I'll try to figure it out at work tomorrow.

threepointone added some commits Jan 14, 2019

review changes
- numbered enum for hook types
- s/hookType/_debugType
- better dce
@threepointone

This comment has been minimized.

Copy link
Contributor Author

threepointone commented Jan 14, 2019

thanks for reviewing my PR on a sunday y'all :)

threepointone added some commits Jan 14, 2019

@threepointone

This comment has been minimized.

Copy link
Contributor Author

threepointone commented Jan 14, 2019

a couple that are missing

  • useImperativeHandle
  • useContext

EDIT: done

nulling currentHookType
need to verify dce still works
@aweary

This comment has been minimized.

Copy link
Member

aweary commented Jan 17, 2019

Looks like there's some overlap here with #14594 for tracking the hook name/type in DEV

@threepointone

This comment has been minimized.

Copy link
Contributor Author

threepointone commented Jan 17, 2019

@aweary hrmm yeah you're right. I'll refactor. thanks for the catch!

@threepointone

This comment has been minimized.

Copy link
Contributor Author

threepointone commented Jan 21, 2019

I made a version where it would show a side by side diff of hook trees using debug-tools (incl. custom hooks), but it turned out to be too invasive and might be more trouble than it's worth (and you have to already have broken the linter by this point.) so pulled that out, rewrote this diff to show the side by side thing for low level hooks. I'll adjust a screenshot soonly.

@threepointone

This comment has been minimized.

Copy link
Contributor Author

threepointone commented Jan 21, 2019

How's this -
screenshot 2019-01-21 at 18 32 48

threepointone added some commits Jan 21, 2019

Show resolved Hide resolved packages/react-reconciler/src/ReactFiberHooks.js Outdated
Show resolved Hide resolved packages/react-reconciler/src/ReactFiberHooks.js Outdated
Show resolved Hide resolved packages/react-reconciler/src/ReactFiberHooks.js Outdated
Show resolved Hide resolved packages/react-reconciler/src/ReactFiberHooks.js Outdated
const ImperativeHandleHook = 8;
const DebugValueHook = 9;

const HookDevNames = [

This comment has been minimized.

@gaearon

gaearon Jan 21, 2019

Member

I'd write these as switch so that the 1:1 correspondence is clear, and also so that we can (type) check if is exhaustive, HookType -> string

This comment has been minimized.

@threepointone

threepointone Jan 21, 2019

Author Contributor

ugh I missed this, will get to in a bit

This comment has been minimized.

@threepointone

threepointone Jan 22, 2019

Author Contributor

I thought about this, and maybe I don't think this is critical? we're not planning on changing this list anytime soon, and it'll add complexity.

This comment has been minimized.

@gaearon

gaearon Jan 22, 2019

Member

Not saying this is critical but I'm also not sure how this will add complexity.

This comment has been minimized.

@threepointone

threepointone Jan 22, 2019

Author Contributor

didn't want to add extra function calls and the comparisons when it was readily indexed and available here.

Show resolved Hide resolved packages/react-reconciler/src/ReactFiberHooks.js Outdated
Show resolved Hide resolved packages/react-reconciler/src/ReactFiberHooks.js Outdated
@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Jan 21, 2019

Proposal for output format:

function useCustom() {
  useState()
  useEffect()
}

function useCustom2() {
  useState()
}

function App() {
  useState();
  useEffect();
  if (foo) {
    useCustom1()
  }
  useCustom2()
  useReducer()
  useState()
}
Previous render      Next render
--------------------------------
1. useState             useState
2. useEffect            useEffect
3. useState             useState
4. useReducer           useEffect
   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
The first Hook type mismatch occurred:
  at useEffect (Foo:xx)
  at useCustom2 (Foo:xx)
  at App (Foo:xx)

This error occurred in the following component:
  in App
@threepointone

This comment has been minimized.

Copy link
Contributor Author

threepointone commented Jan 21, 2019

done
screenshot 2019-01-21 at 20 52 22

threepointone added some commits Jan 21, 2019

@acdlite
Copy link
Member

acdlite left a comment

All of these are nits except the WeakSet question. Let's get rid of that (unless I'm wrong and you actually do need it) and then we'll be good to go.

Show resolved Hide resolved packages/react-reconciler/src/ReactFiberHooks.js
Show resolved Hide resolved packages/react-reconciler/src/ReactFiberHooks.js Outdated
Show resolved Hide resolved packages/react-reconciler/src/ReactFiberHooks.js Outdated
Show resolved Hide resolved packages/react-reconciler/src/ReactFiberHooks.js Outdated
@threepointone

This comment has been minimized.

Copy link
Contributor Author

threepointone commented Jan 22, 2019

alright, addressed all the feedback, and also managed to remove currentHookType, just using currentHookNameInDev. It does feel a bit nicer now.

@acdlite
Copy link
Member

acdlite left a comment

Rest are nits. Thanks for being so patient with all the feedback :) Really appreciate it

Show resolved Hide resolved packages/react-reconciler/src/ReactFiberHooks.js Outdated
Show resolved Hide resolved packages/react-reconciler/src/ReactFiberHooks.js Outdated
@threepointone

This comment has been minimized.

Copy link
Contributor Author

threepointone commented Jan 22, 2019

alright, this has been a long journey, thanks everyone for coming along. I'll merge this in a bit.

2rscr2

@threepointone threepointone merged commit ecd919a into facebook:master Jan 22, 2019

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

jetoneza added a commit to jetoneza/react that referenced this pull request Jan 23, 2019

RFC: warn when returning different hooks on subsequent renders (faceb…
…ook#14585)

* warn when returning different hooks on next render

like it says. adds a field to Hook to track effect 'type', and compares when cloning subsequently.

* lint

* review changes

- numbered enum for hook types
- s/hookType/_debugType
- better dce

* cleaner detection location

* redundant comments

* different EffectHook / LayoutEffectHook

* prettier

* top level currentHookType

* nulling currentHookType

need to verify dce still works

* small enhancements

* hook order checks for useContext/useImperative

* prettier

* stray whitespace

* move some bits around

* better errors

* pass tests

* lint, flow

* show a before - after diff

* an error stack in the warning

* lose currentHookMatches, fix a test

* tidy

* clear the mismatch only in dev

* pass flow

* side by side diff

* tweak warning

* pass flow

* dedupe warnings per fiber, nits

* better format

* nit

* fix bad merge, pass flow

* lint

* missing hooktype enum

* merge currentHookType/currentHookNameInDev, fix nits

* lint

* final nits

n8schloss added a commit to n8schloss/react that referenced this pull request Jan 31, 2019

RFC: warn when returning different hooks on subsequent renders (faceb…
…ook#14585)

* warn when returning different hooks on next render

like it says. adds a field to Hook to track effect 'type', and compares when cloning subsequently.

* lint

* review changes

- numbered enum for hook types
- s/hookType/_debugType
- better dce

* cleaner detection location

* redundant comments

* different EffectHook / LayoutEffectHook

* prettier

* top level currentHookType

* nulling currentHookType

need to verify dce still works

* small enhancements

* hook order checks for useContext/useImperative

* prettier

* stray whitespace

* move some bits around

* better errors

* pass tests

* lint, flow

* show a before - after diff

* an error stack in the warning

* lose currentHookMatches, fix a test

* tidy

* clear the mismatch only in dev

* pass flow

* side by side diff

* tweak warning

* pass flow

* dedupe warnings per fiber, nits

* better format

* nit

* fix bad merge, pass flow

* lint

* missing hooktype enum

* merge currentHookType/currentHookNameInDev, fix nits

* lint

* final nits

Kiku-git added a commit to Kiku-git/react that referenced this pull request Feb 10, 2019

RFC: warn when returning different hooks on subsequent renders (faceb…
…ook#14585)

* warn when returning different hooks on next render

like it says. adds a field to Hook to track effect 'type', and compares when cloning subsequently.

* lint

* review changes

- numbered enum for hook types
- s/hookType/_debugType
- better dce

* cleaner detection location

* redundant comments

* different EffectHook / LayoutEffectHook

* prettier

* top level currentHookType

* nulling currentHookType

need to verify dce still works

* small enhancements

* hook order checks for useContext/useImperative

* prettier

* stray whitespace

* move some bits around

* better errors

* pass tests

* lint, flow

* show a before - after diff

* an error stack in the warning

* lose currentHookMatches, fix a test

* tidy

* clear the mismatch only in dev

* pass flow

* side by side diff

* tweak warning

* pass flow

* dedupe warnings per fiber, nits

* better format

* nit

* fix bad merge, pass flow

* lint

* missing hooktype enum

* merge currentHookType/currentHookNameInDev, fix nits

* lint

* final nits
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment