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

[eslint-plugin-react-hooks] Report constant constructions #19590

Merged
merged 9 commits into from
Aug 13, 2020

Conversation

captbaritone
Copy link
Contributor

Summary

This PR expands upon the existing logic which will report functions defined inside the render function as invalid hook dependencies.

We can now detect additional types of expressions which will result in values that will be referentially unique on each render, such as [] or {}.

function MyComponent() {
  const foo = ['a', 'b', c']; // <== This array is reconstructed each render
  const normalizedFoo = useMemo(() => foo.map(expensiveMapper), [foo]);
  return <OtherComponent foo={normalizedFoo} />
}

We also detect cases where an expression may conditionally result in a referentially unique on each render, such as someArr ?? [] or someObj || {}.

function MyComponent({ nullableValue }) {
  const foo = nullableValue ?? ['a', 'b', c']; // <== This array is _potentially_ reconstructed each render
  const normalizedFoo = useMemo(() => foo.map(expensiveMapper), [foo]);
  return <OtherComponent foo={normalizedFoo} />
}

Test Plan

yarn jest

The dependency array passed to a React hook can be thought of as a list of cache keys. On each render, if any dependency is not `===` its previous value, the hook will be rerun. Constructing a new object/array/function/etc directly within your render function means that the value will be referentially unique on each render. If you then use that value as a hook dependency, that hook will get a "cache miss" on every render, making the dependency array useless.

This can be especially dangerous since it can cascade. If a hook such as `useMemo` is rerun on each render, not only are we bypassing the option to avoid potentially expensive work, but the value _returned_ by `useMemo` may end up being referentially unique on each render causing other downstream hooks or memoized components to become deoptimized.
@codesandbox-ci
Copy link

codesandbox-ci bot commented Aug 12, 2020

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 31f75cd:

Sandbox Source
React Configuration

getConstructionExpresionType(node.left) != null ||
getConstructionExpresionType(node.right) != null
) {
return 'logical expression';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For even better developer experience, we could try to blame the actual sub-expression that is a construction.

For example:

const foo = bar ?? [];
                   ^^ This is actually to blame

However, it would add some additional complexity.

@captbaritone
Copy link
Contributor Author

FYI @gaearon who suggested we add this here rather than as a separate rule.

cc @dylanOshima who paired with me on the original, standalone, version of this rule.

@sizebot
Copy link

sizebot commented Aug 12, 2020

Details of bundled changes.

Comparing: c8d9b88...31f75cd

eslint-plugin-react-hooks

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
eslint-plugin-react-hooks.development.js +3.2% +3.5% 84.38 KB 87.1 KB 19.36 KB 20.03 KB NODE_DEV
eslint-plugin-react-hooks.production.min.js 🔺+4.8% 🔺+4.7% 23.31 KB 24.43 KB 7.89 KB 8.26 KB NODE_PROD

Size changes (stable)

Generated by 🚫 dangerJS against 31f75cd

@sizebot
Copy link

sizebot commented Aug 12, 2020

Details of bundled changes.

Comparing: c8d9b88...31f75cd

eslint-plugin-react-hooks

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
eslint-plugin-react-hooks.development.js +3.2% +3.5% 84.4 KB 87.11 KB 19.37 KB 20.04 KB NODE_DEV
eslint-plugin-react-hooks.production.min.js 🔺+4.8% 🔺+4.7% 23.33 KB 24.44 KB 7.9 KB 8.27 KB NODE_PROD

Size changes (experimental)

Generated by 🚫 dangerJS against 31f75cd

@captbaritone
Copy link
Contributor Author

The ExhaustiveDeps test was segfaulting on my work machine unless I omitted most of the tests, so I was only running my new tests (using only: true). They seem to run fine on my personal computer, so I'll fix up failing tests now.

@captbaritone captbaritone changed the title [eslint-plugin-react-cooks] Report constant constructions [eslint-plugin-react-hooks] Report constant constructions Aug 12, 2020
@captbaritone
Copy link
Contributor Author

captbaritone commented Aug 12, 2020

I got this working on VSCode with some actual production code and looked at a few examples. Some thoughts:

Suggestions are helpful but clunky

If I have a conditional array construction in my render function the flow looks like this:

  1. Start state:
  • const foo = bar ?? [];
  1. See the error and apply the automatic suggestion to wrap in useMemo:
  • const foo = useMemo(() => {return bar ?? [];});
  1. Realize that useMemo is not imported. Manually import that (auto import screwed up due to required/import migration)
  • import {useMemo} from 'React';
  1. Lint complains about missing dependency array, but does not suggest a fix. Manually add an empty one:
  • const foo = useMemo(() => {return bar ?? [];}, []);
  1. Lint informs me I'm missing a dependency bar and offers an automatic fix, which I take:
  • const foo = useMemo(() => {return bar ?? [];}, [bar]);

Overall this is clunky, but at least I get an error at each point and the error tells me what to do.

Should our suggested fix include an empty dependency array? That changes the semantics, but gets the user closer to a fix, assuming they listen to other lint advice.

Wrapping only the declaration may cause bugs if the value gets mutated

Consider this case:

function Greeting({name}) {
  const myArr = ['Hello'];
  if (name) {
    myArr.push(name);
  }
  const greeting = useMemo(() => myArr.join(' '), [myArr]);
  return greeting;
}

Currently we offer an automatic suggestion to wrap the declaration in useMemo, but that would cause a bug:

function Greeting({name}) {
  const myArr = useMemo(() => ['Hello'];
  if (name) { // <== ERROR: `name` could change and `greeting` would not update.
    myArr.push(name);
  }
  const greeting = useMemo(() => myArr.join(' '), [myArr]);
  return greeting;
}

Maybe we need to check for the case where the value gets mutated and not offer an automated fix in that case

@gaearon
Copy link
Collaborator

gaearon commented Aug 12, 2020

Should our suggested fix include an empty dependency array?

Can we suggest a filled-in array in the first place?

@gaearon
Copy link
Collaborator

gaearon commented Aug 12, 2020

Currently we offer an automatic suggestion to wrap the declaration in useMemo, but that would cause a bug:

Yeah — this is really, really bad.

How do you feel about starting without an autofix at all? We can maybe add it later if it's too burdensome.

@captbaritone
Copy link
Contributor Author

How do you feel about starting without an autofix at all? We can maybe add it later if it's too burdensome.

Sounds good. Should we leave the autofix for functions on the assumption that it's not very common for folks to mutate functions?

@captbaritone
Copy link
Contributor Author

Can we suggest a filled-in array in the first place?

I'm sure we could, I haven't looked hard enough to know exactly how much work it would be. I saw this comment (maybe you wrote it?) and just assumed it would be more work than was worth it for now.

// TODO: ideally we'd gather deps here but it would require
// restructuring the rule code. This will cause a new lint
// error to appear immediately for useCallback. Note we're
// not adding [] because would that changes semantics.

@gaearon
Copy link
Collaborator

gaearon commented Aug 12, 2020

Should we leave the autofix for functions on the assumption that it's not very common for folks to mutate functions?

Yeah I think that's reasonable to keep.

Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

Let's remove the risky useMemo wrapping for new cases

@captbaritone
Copy link
Contributor Author

@gaearon Actually, I'm trying to think how hard it would be to detect cases where the value might have been mutated. Would it be sufficient to identify cases where the object has one of its properties read/written/called? If it's that simple, and if the ESLint scope API can give us that info easily (I'm still new to that API and it's poorly documented) then maybe it would be possible to allow the fix in the common case where the value is not mutated.

@gaearon
Copy link
Collaborator

gaearon commented Aug 12, 2020

One way to tell is to go through references and see what its usage is like. It might work. But keep in mind it might be tricky to determine. Like if an array is used with arr.map() for rendering, we wouldn't know with a simple heuristic if this mutates or not.

It may not be safe to just wrap the declaration of an object, since the object may get mutated.

Only offer this autofix for functions which are unlikely to get mutated.

Also, update the message to clarify that the entire construction of the value should get wrapped.
@captbaritone
Copy link
Contributor Author

Updated:

  • Only offer this autofix for functions -- which are unlikely to get mutated.
  • Update the message to clarify that the entire construction of the value should get wrapped not just the declaration.

@captbaritone
Copy link
Contributor Author

I'll punt on trying to detect potential ussages for now.

@gaearon
Copy link
Collaborator

gaearon commented Aug 12, 2020

Gonna review this tomorrow morning.

) {
suggest = [
{
desc: `Wrap the construction of '${construction.name.name}' in its own ${wrapperHook}() Hook.`,

Choose a reason for hiding this comment

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

Other options instead of construction:

  • instantiation
  • initialisation (add a z if you want to americanise it 😉)
  • assignment

Comment on lines +891 to +894
const [before, after] =
wrapperHook === 'useMemo'
? [`useMemo(() => { return `, '; })']
: ['useCallback(', ')'];

Choose a reason for hiding this comment

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

nit - could pull this up to L857 where you declare wrapperHook

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this is only used within the actual fix, it seemed reasonable to have it here.

Comment on lines 1430 to 1431
return null;
}

Choose a reason for hiding this comment

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

I believe that you could also add support for JSX here, because under the hood they are going to create a new object ref each render (right? I don't know the exact semantics of JSX transforms)

const frag = <></>; // JSXFragment
const comp = <div />; // JSXElement
useMemo(() => {}, [frag, comp]);

Copy link

@bradzacher bradzacher Aug 12, 2020

Choose a reason for hiding this comment

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

could also add handling for AssignmentExpression:

let x = null;
let y = x = () => {};
useMemo(() => {}, [y]);

(i.e. ignore the node and check the right)

Choose a reason for hiding this comment

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

could also add handling for NewExpression?

const x = new Map();
useMemo(() => {}, [x]);

Choose a reason for hiding this comment

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

could also add handling for Literal when value instanceof RegExp?

const x = /a/;
useMemo(() => {}, [x]);

Choose a reason for hiding this comment

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

could also handle TS type assertions (TSAsExpression):

const x = {} as any;
useMemo(() => {}, [x]);

(i.e. ignore the node and check the expression.)

Choose a reason for hiding this comment

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

could also handle flow type casts (TypeCastExpression):

const x = ({}: any);
useMemo(() => {}, [x]);

(i.e. ignore the node and check the expression.)

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 list is gold. Thanks! I had intentionally omitted NewExpression thinking that String Number and Boolean would be compared by value, but I forgot that they will be boxed so presumably they will be referentially unique and therefore worth flagging.

Choose a reason for hiding this comment

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

Yeah I didn't think about that, but testing in chrome shows they are referentially unique
image

Note that they're also not even loosely equal:

image

(note: if you swap one side for a literal, it coerces the object to the same type and then they become loosely equal)

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is such an edge case btw. I don't think handling boxed primitives is super important.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gaearon I would agree that it's very unlikely to happen, we get it for free when handling other NewExpressions like new Map([]).

@gaearon
Copy link
Collaborator

gaearon commented Aug 13, 2020

You'll want to add some tests for cases where the same variable is used by multiple useMemo's.

@captbaritone
Copy link
Contributor Author

@gaearon I've added a test for a constant construction used by multiple dependency arrays here: 31f75cd

Note that it creates two different errors at the same location. One referencing the first dependency array, and one referencing the other.

@gaearon
Copy link
Collaborator

gaearon commented Aug 13, 2020

For patterns where an object doesn't use any values from the pure scopes, would it be possible to change the message to suggest to move it outside the component scope? This is a lot less noisy and seems fairly common in practice.

@captbaritone
Copy link
Contributor Author

For patterns where an object doesn't use any values from the pure scopes, would it be possible to change the message to suggest to move it outside the component scope? This is a lot less noisy and seems fairly common in practice.

Good suggestion, but I think it would require significantly more analysis. Currently we don't do any work to try to understand the values used to construct the object. If we did, not only could we adjust the message as you suggest, but we could also construct the dependency array for the autofix of adding a wrapping useMemo. Given the amount of work involved, I think it makes sense to leave this for another task.

@gaearon gaearon merged commit 1d5e10f into facebook:master Aug 13, 2020
@nstepien
Copy link

Will it also handle cases like default props? e.g.:

function Foo({ arr = [] }) {
  useMemo(() => {
    console.log(arr);
  }, [arr]);
}

@captbaritone
Copy link
Contributor Author

@nstepien Good question. It doesn't currently. It looks like it wouldn't be too hard to add though. Here's a commit that adds it: 29d560b

I'm happy to open that as a PR, but I suspect we would want to come up with a somewhat different error message in that case, and I'm not sure I have the time right now to work on that (maybe you do?).

Thoughts @gaearon? Do you agree that we would want to different error message for the default param case?

@gaearon
Copy link
Collaborator

gaearon commented Aug 14, 2020

That would need a different message, yes.

facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Oct 27, 2020
Summary:
This sync includes the following changes:
- **[eaaf4cbce](facebook/react@eaaf4cbce )**: 17.0.1 //<Dan Abramov>//
- **[6f62abb58](facebook/react@6f62abb58 )**: Remove usage of Array#fill ([#20071](facebook/react#20071)) //<Dan Abramov>//
- **[40cfe1f48](facebook/react@40cfe1f48 )**: Update CHANGELOG.md //<Dan Abramov>//
- **[f021a983a](facebook/react@f021a983a )**: Bump versions for 17 ([#20062](facebook/react#20062)) //<Dan Abramov>//
- **[d1bb4d851](facebook/react@d1bb4d851 )**: Profiler: Include ref callbacks in onCommit duration ([#20060](facebook/react#20060)) //<Brian Vaughn>//
- **[c59c3dfe5](facebook/react@c59c3dfe5 )**: useRef: Warn about reading or writing mutable values during render ([#18545](facebook/react#18545)) //<Brian Vaughn>//
- **[7b6cac952](facebook/react@7b6cac952 )**: Improved Profiler commit hooks test ([#20053](facebook/react#20053)) //<Brian Vaughn>//
- **[dfb6a4033](facebook/react@dfb6a4033 )**: [Fast Refresh] Fix crashes caused by rogue Proxies ([#20030](facebook/react#20030)) ([#20039](facebook/react#20039)) //<Kai Riemann>//
- **[37cb732c5](facebook/react@37cb732c5 )**: Use bitwise OR to define flag masks ([#20044](facebook/react#20044)) //<Andrew Clark>//
- **[eb3181e77](facebook/react@eb3181e77 )**: Add Visibility flag for hiding/unhiding trees ([#20043](facebook/react#20043)) //<Andrew Clark>//
- **[0dd809bdf](facebook/react@0dd809bdf )**: Remove last schedulePassiveEffectCallback call ([#20042](facebook/react#20042)) //<Andrew Clark>//
- **[8df7b7911](facebook/react@8df7b7911 )**: Remove Passive flag from "before mutation" phase ([#20038](facebook/react#20038)) //<Andrew Clark>//
- **[c57fe4a2c](facebook/react@c57fe4a2c )**: ReactIs.isValidElementType Unit Test extended with PureComponent case ([#20033](facebook/react#20033)) //<adasq>//
- **[02da938fd](facebook/react@02da938fd )**: Don't double-invoke effects in legacy roots ([#20028](facebook/react#20028)) //<Brian Vaughn>//
- **[d95c4938d](facebook/react@d95c4938d )**: [EventSystem] Revise onBeforeBlur propagation mechanics ([#20020](facebook/react#20020)) //<Dominic Gannaway>//
- **[f46a80ae1](facebook/react@f46a80ae1 )**: Update outdated links and fix two broken links  ([#19985](facebook/react#19985)) //<Saikat Guha>//
- **[0a4c7c565](facebook/react@0a4c7c565 )**: [Flight] Don't warn for key, but error for ref ([#19986](facebook/react#19986)) //<Sebastian Markbåge>//
- **[993ca533b](facebook/react@993ca533b )**: Enable eager listeners statically ([#19983](facebook/react#19983)) //<Dan Abramov>//
- **[40c52de96](facebook/react@40c52de96 )**: [Flight] Add Runtime Errors for Non-serializable Values ([#19980](facebook/react#19980)) //<Sebastian Markbåge>//
- **[1992d9730](facebook/react@1992d9730 )**: Revert "Temporarily disable Profiler commit hooks flag ([#19900](facebook/react#19900))" ([#19960](facebook/react#19960)) //<Brian Vaughn>//
- **[44d39c4d7](facebook/react@44d39c4d7 )**: Removed skip-error-boundaries modifications from old fork ([#19961](facebook/react#19961)) //<Brian Vaughn>//
- **[cc77be957](facebook/react@cc77be957 )**: Remove unnecessary error overriding in ([#19949](facebook/react#19949)) //<Paul Doyle>//
- **[97625272a](facebook/react@97625272a )**: Debug tracing tests for CPU bound suspense ([#19943](facebook/react#19943)) //<Brian Vaughn>//
- **[43363e279](facebook/react@43363e279 )**: Fix codestyle for typeof comparison ([#19928](facebook/react#19928)) //<Eugene Maslovich>//
- **[5427b4657](facebook/react@5427b4657 )**: Temporarily disable Profiler commit hooks flag ([#19900](facebook/react#19900)) //<Brian Vaughn>//
- **[1faf9e3dd](facebook/react@1faf9e3dd )**: Suspense for CPU-bound trees ([#19936](facebook/react#19936)) //<Andrew Clark>//
- **[7f08e908b](facebook/react@7f08e908b )**: Fix missing context to componentDidMount() when double-invoking lifecycles ([#19935](facebook/react#19935)) //<Brian Vaughn>//
- **[9198a5cec](facebook/react@9198a5cec )**: Refactor layout effect methods ([#19895](facebook/react#19895)) //<Brian Vaughn>//
- **[ba82eea38](facebook/react@ba82eea38 )**: Remove disableSchedulerTimeoutInWorkLoop flag ([#19902](facebook/react#19902)) //<Andrew Clark>//
- **[c63741fb3](facebook/react@c63741fb3 )**: offscreen double invoke effects ([#19523](facebook/react#19523)) //<Luna Ruan>//
- **[c6917346f](facebook/react@c6917346f )**: Fixed broken Profiler test ([#19894](facebook/react#19894)) //<Brian Vaughn>//
- **[87c023b1c](facebook/react@87c023b1c )**: Profiler onRender only called when we do work ([#19885](facebook/react#19885)) //<Brian Vaughn>//
- **[81aaee56a](facebook/react@81aaee56a )**: Don't call onCommit et al if there are no effects ([#19863](facebook/react#19863)) //<Andrew Clark>//
- **[7355bf575](facebook/react@7355bf575 )**: Consolidate commit phase hook functions ([#19864](facebook/react#19864)) //<Andrew Clark>//
- **[bc6b7b6b1](facebook/react@bc6b7b6b1 )**: Don't trigger lazy in DEV during element creation ([#19871](facebook/react#19871)) //<Dan Abramov>//
- **[a774502e0](facebook/react@a774502e0 )**: Use single quotes in getComponentName return ([#19873](facebook/react#19873)) //<Gustavo Saiani>//
- **[8b2d3783e](facebook/react@8b2d3783e )**: Use Passive flag to schedule onPostCommit ([#19862](facebook/react#19862)) //<Andrew Clark>//
- **[50d9451f3](facebook/react@50d9451f3 )**: Improve DevTools editing interface ([#19774](facebook/react#19774)) //<Brian Vaughn>//
- **[6fddca27e](facebook/react@6fddca27e )**: Remove passive intervention flag ([#19849](facebook/react#19849)) //<Dan Abramov>//
- **[36df9185c](facebook/react@36df9185c )**: chore(docs): Removed outdated comment about fb.me link  ([#19830](facebook/react#19830)) //<Adnaan Bheda>//
- **[16fb2b6f9](facebook/react@16fb2b6f9 )**: Moved resetChildLanes into complete work ([#19836](facebook/react#19836)) //<Brian Vaughn>//
- **[cc581065d](facebook/react@cc581065d )**: eslint-plugin-react-hooks@4.1.2 //<Dan Abramov>//
- **[0044805c8](facebook/react@0044805c8 )**: Update CHANGELOG.md //<Dan Abramov>//
- **[0f70d4dd6](facebook/react@0f70d4dd6 )**: Consider components in jsx as missing dependencies in typescript-eslint/parser@4.x ([#19815](facebook/react#19815)) //<Sebastian Silbermann>//
- **[84558c61b](facebook/react@84558c61b )**: Don't visit passive effects during layout phase ([#19809](facebook/react#19809)) //<Andrew Clark>//
- **[ad8a0a8cd](facebook/react@ad8a0a8cd )**: eslint-plugin-react-hooks@4.1.1 //<Dan Abramov>//
- **[77544a0d6](facebook/react@77544a0d6 )**: Update CHANGELOG.md //<Dan Abramov>//
- **[ed4fdfc73](facebook/react@ed4fdfc73 )**: test(eslint-plugin-react-hooks): Run with TS parsers >= 2.x ([#19792](facebook/react#19792)) //<Sebastian Silbermann>//
- **[cd75f93c0](facebook/react@cd75f93c0 )**: eslint-plugin-react-hooks: fix compatibility with typescript-eslint/parser@4.0.0+ ([#19751](facebook/react#19751)) //<Matthias Schiffer>//
- **[781212aab](facebook/react@781212aab )**: Remove double space in test name ([#19762](facebook/react#19762)) //<Gustavo Saiani>//
- **[e7b255341](facebook/react@e7b255341 )**: Internal `act`: Flush timers at end of scope ([#19788](facebook/react#19788)) //<Andrew Clark>//
- **[d17086c7c](facebook/react@d17086c7c )**: Decouple public, internal act implementation ([#19745](facebook/react#19745)) //<Andrew Clark>//
- **[d38ec17b1](facebook/react@d38ec17b1 )**: [Flight] Set dispatcher for duration of performWork() ([#19776](facebook/react#19776)) //<Joseph Savona>//
- **[4f3f7eeb7](facebook/react@4f3f7eeb7 )**: Bugfix: Effect clean up when deleting suspended tree ([#19752](facebook/react#19752)) //<Andrew Clark>//
- **[7baf9d412](facebook/react@7baf9d412 )**: Combine Flags and SubtreeFlags types ([#19775](facebook/react#19775)) //<Andrew Clark>//
- **[166544360](facebook/react@166544360 )**: Rename effect fields ([#19755](facebook/react#19755)) //<Andrew Clark>//
- **[708fa77a7](facebook/react@708fa77a7 )**: Decrease expiration time of input updates ([#19772](facebook/react#19772)) //<Andrew Clark>//
- **[36df483af](facebook/react@36df483af )**: Add feature flag to disable scheduler timeout in work loop ([#19771](facebook/react#19771)) //<Ricky>//
- **[bcc0aa463](facebook/react@bcc0aa463 )**: Revert "Revert "Remove onScroll bubbling flag ([#19535](facebook/react#19535))" ([#19655](facebook/react#19655))" ([#19761](facebook/react#19761)) //<Dan Abramov>//
- **[99cae887f](facebook/react@99cae887f )**: Add failing test for passive effect cleanup functions and memoized components ([#19750](facebook/react#19750)) //<Brian Vaughn>//
- **[2cfd73c4d](facebook/react@2cfd73c4d )**: Fix typo in comment (Noticable→Noticeable) ([#19737](facebook/react#19737)) //<Ikko Ashimine>//
- **[53e622ca7](facebook/react@53e622ca7 )**: Fix instances of function declaration after return ([#19733](facebook/react#19733)) //<Bhumij Gupta>//
- **[b7d18c4da](facebook/react@b7d18c4da )**: Support Babel's envName option in React Refresh plugin ([#19009](facebook/react#19009)) //<Kevin Weber>//
- **[1f38dcff6](facebook/react@1f38dcff6 )**: Remove withSuspenseConfig ([#19724](facebook/react#19724)) //<Andrew Clark>//
- **[1396e4a8f](facebook/react@1396e4a8f )**: Fixes eslint warning when node type is ChainExpression ([#19680](facebook/react#19680)) //<Pascal Fong Kye>//
- **[a8500be89](facebook/react@a8500be89 )**: Add `startTransition` as a known stable method ([#19720](facebook/react#19720)) //<Andrew Clark>//
- **[380dc95de](facebook/react@380dc95de )**: Revert "Append text string to <Text> error message ([#19581](facebook/react#19581))" ([#19723](facebook/react#19723)) //<Timothy Yung>//
- **[ddd1faa19](facebook/react@ddd1faa19 )**: Remove config argument from useTransition ([#19719](facebook/react#19719)) //<Andrew Clark>//
- **[92fcd46cc](facebook/react@92fcd46cc )**: Replace SuspenseConfig object with an integer ([#19706](facebook/react#19706)) //<Andrew Clark>//
- **[b754caaaf](facebook/react@b754caaaf )**: Enable eager listeners in open source ([#19716](facebook/react#19716)) //<Dan Abramov>//
- **[c1ac05215](facebook/react@c1ac05215 )**: [Flight] Support more element types and Hooks for Server and Hybrid Components ([#19711](facebook/react#19711)) //<Dan Abramov>//
- **[1eaafc9ad](facebook/react@1eaafc9ad )**: Clean up timeoutMs-related implementation details ([#19704](facebook/react#19704)) //<Andrew Clark>//
- **[8da0da093](facebook/react@8da0da093 )**: Disable timeoutMs argument ([#19703](facebook/react#19703)) //<Andrew Clark>//
- **[60ba723bf](facebook/react@60ba723bf )**: Add SuspenseList to devTools ([#19684](facebook/react#19684)) //<Ben Pernick>//
- **[5564f2c95](facebook/react@5564f2c95 )**: Add React.startTransition ([#19696](facebook/react#19696)) //<Ricky>//
- **[c4e0768d7](facebook/react@c4e0768d7 )**: Remove unused argument from `finishConcurrentRender` ([#19689](facebook/react#19689)) //<inottn>//
- **[848bb2426](facebook/react@848bb2426 )**: Attach Listeners Eagerly to Roots and Portal Containers ([#19659](facebook/react#19659)) //<Dan Abramov>//
- **[d2e914ab4](facebook/react@d2e914ab4 )**: Remove remaining references to effect list ([#19673](facebook/react#19673)) //<Andrew Clark>//
- **[d6e433899](facebook/react@d6e433899 )**: Use Global Render Timeout for CPU Suspense ([#19643](facebook/react#19643)) //<Sebastian Markbåge>//
- **[64ddef44c](facebook/react@64ddef44c )**: Revert "Remove onScroll bubbling flag ([#19535](facebook/react#19535))" ([#19655](facebook/react#19655)) //<Dan Abramov>//
- **[dd651df05](facebook/react@dd651df05 )**: Keep onTouchStart, onTouchMove, and onWheel passive ([#19654](facebook/react#19654)) //<Dan Abramov>//
- **[b8fa09e9e](facebook/react@b8fa09e9e )**: provide profiling bundle for react-reconciler ([#19559](facebook/react#19559)) //<Julien Gilli>//
- **[23595ff59](facebook/react@23595ff59 )**: Add missing param to safelyCallDestroy() ([#19638](facebook/react#19638)) //<Brian Vaughn>//
- **[ee409ea3b](facebook/react@ee409ea3b )**: change destroy to safelyCallDestroy ([#19605](facebook/react#19605)) //<Luna Ruan>//
- **[bcca5a6ca](facebook/react@bcca5a6ca )**: Always skip unmounted/unmounting error boundaries ([#19627](facebook/react#19627)) //<Brian Vaughn>//
- **[1a41a196b](facebook/react@1a41a196b )**: Append text string to <Text> error message ([#19581](facebook/react#19581)) //<Timothy Yung>//
- **[e4afb2fdd](facebook/react@e4afb2fdd )**: eslint-plugin-react-hooks@4.1.0 //<Dan Abramov>//
- **[ced05c46c](facebook/react@ced05c46c )**: Update CHANGELOG.md //<Dan Abramov>//
- **[702fad4b1](facebook/react@702fad4b1 )**: refactor fb.me redirect link to reactjs.org/link ([#19598](facebook/react#19598)) //<CY Lim>//
- **[49cd77d24](facebook/react@49cd77d24 )**: fix: leak strict mode with UMD builds ([#19614](facebook/react#19614)) //<Toru Kobayashi>//
- **[ffb749c95](facebook/react@ffb749c95 )**: Improve error boundary handling for unmounted subtrees ([#19542](facebook/react#19542)) //<Brian Vaughn>//
- **[9b35dd2fc](facebook/react@9b35dd2fc )**: Permanently removed component stacks from scheduling profiler data ([#19615](facebook/react#19615)) //<Brian Vaughn>//
- **[3f8115cdd](facebook/react@3f8115cdd )**: Remove `didTimeout` check from work loop //<Andrew Clark>//
- **[9abc2785c](facebook/react@9abc2785c )**: Remove wasteful checks from `shouldYield` //<Andrew Clark>//
- **[1d5e10f70](facebook/react@1d5e10f70 )**: [eslint-plugin-react-hooks] Report constant constructions ([#19590](facebook/react#19590)) //<Jordan Eldredge>//
- **[dab0854c5](facebook/react@dab0854c5 )**: Move commit passive unmount/mount to CommitWork ([#19599](facebook/react#19599)) //<Sebastian Markbåge>//
- **[ccb6c3945](facebook/react@ccb6c3945 )**: Remove unused argument ([#19600](facebook/react#19600)) //<inottn>//
- **[629125555](facebook/react@629125555 )**: [Scheduler] Re-throw unhandled errors ([#19595](facebook/react#19595)) //<Andrew Clark>//
- **[b8ed6a1aa](facebook/react@b8ed6a1aa )**: [Scheduler] Call postTask directly ([#19551](facebook/react#19551)) //<Andrew Clark>//
- **[ce37bfad5](facebook/react@ce37bfad5 )**: Remove resolutions from test renderer package.json ([#19577](facebook/react#19577)) //<Dan Abramov>//
- **[2704bb537](facebook/react@2704bb537 )**: Add ReactVersion to SchedulingProfiler render scheduled marks ([#19553](facebook/react#19553)) //<Kartik Choudhary>//
- **[0c52e24cb](facebook/react@0c52e24cb )**: Support inner component _debugOwner in memo ([#19556](facebook/react#19556)) //<Brian Vaughn>//
- **[0cd9a6de5](facebook/react@0cd9a6de5 )**: Parallelize Jest in CI ([#19552](facebook/react#19552)) //<Andrew Clark>//
- **[a63893ff3](facebook/react@a63893ff3 )**: Warn about undefined return value for memo and forwardRef ([#19550](facebook/react#19550)) //<Brian Vaughn>//
- **[32ff42868](facebook/react@32ff42868 )**: Add feature flag for setting update lane priority ([#19401](facebook/react#19401)) //<Ricky>//
- **[5bdd4c8c6](facebook/react@5bdd4c8c6 )**: Remove unused argument from call to jest method ([#19546](facebook/react#19546)) //<Gustavo Saiani>//
- **[a5fed98a9](facebook/react@a5fed98a9 )**: Register more node types that are used later as JSXIdentifiers ([#19514](facebook/react#19514)) //<Mateusz Burzyński>//
- **[f77c7b9d7](facebook/react@f77c7b9d7 )**: Re-add discrete flushing timeStamp heuristic (behind flag) ([#19540](facebook/react#19540)) //<Dominic Gannaway>//

Changelog: [general] [feature] Upgrade to React 17

Reviewed By: cpojer

Differential Revision: D24491201

fbshipit-source-id: c947da9dcccbd614e9dc58f3339b63e24829aca7
@nstepien
Copy link

nstepien commented Nov 12, 2020

@captbaritone Checking for default props is still missing, do you think you could work on it? I can log an issue otherwise.
It should also check for default params in custom hooks, i.e.:

function useArr(arr = []) {
  useEffect(() => {
    console.log(arr);
  }, [arr]);
}

// or

function useArr({ arr = []} ) {
  useEffect(() => {
    console.log(arr);
  }, [arr]);
}

koto pushed a commit to koto/react that referenced this pull request Jun 15, 2021
…9590)

* [eslint-plugin-react-cooks] Report constant constructions

The dependency array passed to a React hook can be thought of as a list of cache keys. On each render, if any dependency is not `===` its previous value, the hook will be rerun. Constructing a new object/array/function/etc directly within your render function means that the value will be referentially unique on each render. If you then use that value as a hook dependency, that hook will get a "cache miss" on every render, making the dependency array useless.

This can be especially dangerous since it can cascade. If a hook such as `useMemo` is rerun on each render, not only are we bypassing the option to avoid potentially expensive work, but the value _returned_ by `useMemo` may end up being referentially unique on each render causing other downstream hooks or memoized components to become deoptimized.

* Fix/remove existing tests

* Don't give an autofix of wrapping object declarations

It may not be safe to just wrap the declaration of an object, since the object may get mutated.

Only offer this autofix for functions which are unlikely to get mutated.

Also, update the message to clarify that the entire construction of the value should get wrapped.

* Handle the long tail of nodes that will be referentially unique

* Catch let/var constant constructions on initial assignment

* Trim trailing whitespace

* Address feedback from @gaearon

* Rename "assignment" to "initialization"

* Add test for a constant construction used in multiple dependency arrays
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.

6 participants