-
Notifications
You must be signed in to change notification settings - Fork 231
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
Adding optional settings to require-hook rule #983
Conversation
28486d4
to
c33f78e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm heading to bed so will give it a proper review tomorrow, but I'm thinking we should just have an allowedFunctions
option, as I think functions taking hooks as their first argument is reasonably specific (I've never come across it myself until now), and if we check for this arguably there could be other patterns people could like us to support (like functions that take the hook as the last argument, if they exist)
Btw I'm pretty sure you can call the method in a But I think it's worth us supporting |
Hey @G-Rath! Thanks for commenting. The first idea came to me is // helpers.js
export default doAllMocks = () => {
jest.mock('oneModule');
jest.mock('anotherModule');
// list of 20 mocks
}
// test.js
import doAllMocks from './helpers';
doAllMocks(); But I'm not sure it is a good practice.
Yes, technically it is good but for me it looks like a little overengineered, like calling Goodnight! I'll be waiting for any decisions 👍 |
@Kreeg I still think we should go with a more generic option as it means less complexity on our side and if we wanted to make it more generic later that'd be a breaking change since we'd be removing an option. e.g. it's perfectly valid (and easy) to do something like this:
This would also make it easier to reuse e.g. for #961, when checking |
@G-Rath it seems reasonable for me. |
c33f78e
to
8405f54
Compare
@G-Rath hey! I updated the branch. I personally don't like meaningless commits so I force pushed it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good - got a couple of minor changes I'd like to see; also, what do you think about using "allow" instead of exclude? e.g. allowedFunctionCalls
.
I feel like that might be a better name since we're allowing functions to be called - exclude to me feels like we're bailing out of checking completely, which isn't the case (we're just not reporting them).
docs/rules/require-hook.md
Outdated
Some test utils provides methods which takes hook as an argument | ||
and should be executed outside a hook. | ||
|
||
For example https://vue-test-utils.vuejs.org/api/#enableautodestroy-hook | ||
which takes the hook as an argument. To exclude them you can update settings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that the methods take hooks as an argument is irrelevant, so this should be say something along the lines of "if there are methods that you want to call outside of hooks and tests, you can mark them as allowed using the allowedFunctionCalls
option" :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the documentation but I still don't like it. Because the misleading option allowedFunctionCalls
, which contain correct function, but the code example is still incorrect because of another function calls. Is it good?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the misleading option allowedFunctionCalls,
I'm not sure I follow you on what you're finding misleading?
I'm happy if you want to play with the documentation some more - do you think it would be better if we didn't have any config in our "incorrect" example (e.g. "Example of incorrect code without using allowedFunctionCalls
"), and left the "correct" example as-is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@G-Rath yes, the problem is just with "incorrect" example. So in docs we are saying "incorrect usage of allowed functions" but there is no incorrect use of these functions in the example. This makes some confusion so I'll remove it
src/rules/require-hook.ts
Outdated
const nodeName = getNodeName(node); | ||
|
||
if (nodeName === null) { | ||
return false; | ||
} | ||
|
||
return !!options.excludedFunctions?.includes(nodeName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to just cast this to a string
, which makes the code a little smaller without any real loss (since this is a known TS limitation that we've got already in a few other places)
const nodeName = getNodeName(node); | |
if (nodeName === null) { | |
return false; | |
} | |
return !!options.excludedFunctions?.includes(nodeName); | |
return !!options.excludedFunctions?.includes(getNodeName(node) as string)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This is my first encounter with TS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem, you're doing good!
@G-Rath Saw your review, I will answer and update later tonight, thank you! |
8405f54
to
ae01775
Compare
ae01775
to
71a0df0
Compare
71a0df0
to
375069f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! Got a few minor nits/recommendations, but nothing blocking - will merge sometime this week :)
@G-Rath I will fix the code due to your suggestions tomorrow, thank you :) |
375069f
to
072b08a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this!
Co-authored-by: Andrey Nelyubin <[email protected]>
(I used |
# [25.3.0](v25.2.4...v25.3.0) (2021-11-23) ### Features * **require-hook:** add `allowedFunctionCalls` setting ([#983](#983)) ([9d9336a](9d9336a))
The problem
Some testing libs provide methods which takes hook as an argument and should be executed outside of a hook. The greatest example is using
enableAutoDestroyHook
from@vue/test-utils
lib .By default
jest/require-hook
rule throws an error in such correct codeTo deal with it you must ignore
jest/require-hook
rule (or mark it as warn). Which is absolutely wrong and causing all the problems described in original rule's docs.The solution
In this PR I added addtional settings for the rule to exclude such tools as
enableAutoDestroy
with additional settings to reduce or maybe extend allowed hooks.