Proposal: react-hooks/exhaustive-deps: a more customizable additionalHooks

[dvoytenko]

Summary

The current additionalHooks in the react-hooks/exhaustive-deps eslint plugin accepts a regular expression to match hook names. It always assumes the following format:

function useCustomHook(callback, deps) {}

However, this format is not always convenient when other arguments are available in the hook's signature, because the set of dependencies could potentially be large. In those cases, it'd be more convenient to place callback and deps as the last two arguments. For instance, the standard useImperativeHandle has an additional first argument:

function useImperativeHandle(ref, callback, deps) {}

To enable this, the proposal adds a new property: additionalHooksMap that takes a map keyed by a hook name and value specifying callback argument offset.

An example:

[{additionalHooksMap: {
    'useCustomEffect': 1,
    'useAnotherCustomEffect': 2,
    ...
}}]

Test Plan

Added new tests for the new format to the eslint tests.

[facebook-github-bot]

Hi @dvoytenko!

Thank you for your pull request and welcome to our community.We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

[codesandbox-ci]

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 f94be69:

Sandbox	Source
silly-moore-r4n3e	Configuration

[codesandbox-ci]

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 d08cd26:

Sandbox	Source
serverless-darkness-j2e3d	Configuration

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[sizebot]

Details of bundled changes.

Comparing: b41beb1...d08cd26

eslint-plugin-react-hooks

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
eslint-plugin-react-hooks.production.min.js	🔺+1.7%	🔺+1.9%	22.87 KB	23.26 KB	7.7 KB	7.84 KB	NODE_PROD
eslint-plugin-react-hooks.development.js	+2.2%	+2.6%	82.28 KB	84.1 KB	18.81 KB	19.3 KB	NODE_DEV

Size changes (stable)

Generated by 🚫 dangerJS against d08cd26

[sizebot]

Details of bundled changes.

Comparing: b41beb1...d08cd26

eslint-plugin-react-hooks

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
eslint-plugin-react-hooks.development.js	+2.2%	+2.6%	82.29 KB	84.11 KB	18.82 KB	19.31 KB	NODE_DEV
eslint-plugin-react-hooks.production.min.js	🔺+1.7%	🔺+1.9%	22.89 KB	23.27 KB	7.7 KB	7.85 KB	NODE_PROD

Size changes (experimental)

Generated by 🚫 dangerJS against d08cd26

[swissspidy]

There's a conflict now because of #18907

[dvoytenko]

Thanks, @swissspidy. Rebased.

[gaearon]

Mind updating? Let's get this in.

[dvoytenko]

@gaearon Rebased. PTAL.

[gaearon]

I think we might want to think more about the API here. What if you want to use a regex and put it in the map? The existing feature supports regexes. It is also a bit confusing to have two independent options. Why does one of them win? What if a regex matches in one but the key matches in the other one?

[gaearon]

Could be something like

additionalHooks: [
  [{ test: /regex/, callbackIndex: 3 }],
  ...
]

[dvoytenko]

I think we might want to think more about the API here. What if you want to use a regex and put it in the map? The existing feature supports regexes. It is also a bit confusing to have two independent options. Why does one of them win? What if a regex matches in one but the key matches in the other one?

Sure. That makes sense. I'll update.

[dvoytenko]

Could be something like

additionalHooks: [
  [{ test: /regex/, callbackIndex: 3 }],
  ...
]

@gaearon PTAL. I updated per your request. The linter schema is now either one of these:

• A string - the current mode.
• An array of objects in the shape {test, callbackIndex}

[dvoytenko]

@gaearon could you ptal to see if this is good to be merged?

[swissspidy]

@gaearon would this be good to go after a rebase?

[swissspidy]

Opened #19644 to track this proposal in case it helps with visibility...

[whatisaphone]

It might be simpler (for end users) to infer the callback index from callsite usage. All the hooks I've ever seen that take a callback have arguments of (...optionalExtraArgs, callback, deps?). So the callback is always second-to-last, or if it's last, the deps array is undefined.

Are there any callback+deps hooks out in the wild that break this assumption?

[dvoytenko]

@whatisaphone I think somewhere this was covered as well. Personally, I always order args to have callback and deps as the last 2 (or 1) args. However, the current behavior is exactly the opposite and so we decided to go with a configurable option for backward compatibility.

[MatthijsBon]

I would like to add that useDebounce has a different signature than what seems to be proposed here:

const [ ... ] = useDebounce(fn: Function, ms: number, deps: DependencyList = []);

I would say that specifying the index of the callback is a non-complete addition and would suggest to instead specify the index of the DependencyList?

[georeith]

@MatthijsBon I think you can assume that dependency list is the last argument. I can see why callback may potentially be moved around but that feels like a pretty solid convention thats worth encouraging.

[stale]

This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.

[dvoytenko]

bump

[andrenanninga]

Any chance of getting this or #21719 merged? Would love to see this feature.

[stale]

This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.

[swissspidy]

bump

[vojtech-dobes]

@gaearon Is there a chance of this being merged and shipped? exhaustive-deps is now very strict and discourages designing a focused API for custom hook, because it's not possible to teach this rule on which position the callback is present.

[kachkaev]

@dvoytenko would you be willing to re-create this PR against the main branch? Seems like this PR is closed because the master branch has been deleted.

[swissspidy]

Quoting #21719 (comment):

That said, I don't think we will be adding support for this. In general, we've been leaning towards recommending that custom Hooks do not take dependency arrays at all, and that you rely on the built-in Hooks to do that instead.