-
Notifications
You must be signed in to change notification settings - Fork 248
ts-migration/improve-guards #316
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
ts-migration/improve-guards #316
Conversation
| node.callee.object.type === AST_NODE_TYPES.Identifier && | ||
| node.callee.object.name in HookName) | ||
| node.callee.type === AST_NODE_TYPES.Identifier && | ||
| node.callee.name in HookName |
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've changed this b/c jest doesn't expect hooks to expose a method property, and this way the two rules that use them don't need to be changed :D
isHook is only used by no-duplicate-hooks and no-hooks.
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 don't follow - you can do describe.skip (etc). Or do I misunderstand your comment?
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.
But describe isn't a hook - the HookName enum is for beforeAll, beforeEach, afterEach & afterAll.
That's why it's the only one I made this change to - the others I left as it b/c in the JS version they have member calls (i.e describe.skip)
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.
hah, good point! There are indeed no identifiers on hooks
I didn't type the guards correctly the first time - see this comment for some more info.
This is something that should be looked into at some point; in JS the type guards are implemented using
getNodeName, which'll give you the following:This is then checked using
#hasagainst aSet, which has since become an enum.However the original
Setcontained both functions and function chains:I've retyped the guards so that they now match the actual runtime checks that are happening, but this means that there is a behavioural difference from the JS version that I think should be commented on.
The behavioural change is that now things like
describe.<anything>()will be causeisDescribeto return true.It's all left me a bit paranoid right now, b/c I've not got enough info to know how big of an impact this'll have on everything - so far I've not had any problems (i.e no tests have failed), but it's technically possible that it means the plugin could try and lint things it shouldn't (which wouldn't necessarily cause an error, since it'll be doing other checks) which we might care about?
This PR should definitely be merged, as it only changes the types to reflect the runtime checks.