-
Notifications
You must be signed in to change notification settings - Fork 55
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
fix: await interactions rule scope. #100
fix: await interactions rule scope. #100
Conversation
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.
Hey @zhyd1997 thanks a lot for your contribution! I added a couple comments.
lib/rules/await-interactions.ts
Outdated
@@ -130,24 +130,36 @@ export = createStorybookRule({ | |||
return getClosestFunctionAncestor(parent) | |||
} | |||
|
|||
const isExpectFromStorybookImported = (node: ImportDeclaration) => { | |||
return ( | |||
node.source.value.startsWith('@storybook/') |
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.
This check will be true if the user has any random import from @storybook/*
such as:
import { actions } from '@storybook/addon-actions'
import { Meta } from '@storybook/react'
import { expect } from '@storybook/jest'
while this is the import that should matter.
import { userEvent } from '@storybook/testing-library'
That is, for the userEvent case in particular. It's still possible to have play functions that do not use @storybook/testing-library but use expect
from @storybook/jest instead
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.
Here are a couple more examples:
// if userEvent is imported from here, then it should provide an error message
import { userEvent } from '@storybook/testing-library'
// if userEvent is imported from somewhere else, it should not
import { userEvent } from '../utils'
Another example that the plugin should not provide an error message:
MyStory.play = async () => {
const userEvent = { test: () => {} }
// should not complain
userEvent.test()
}
And another example that the plugin should provide an error message:
import { expect } from '@storybook/jest'
MyStory.play = async ({ args }) => {
// should complain
expect(args.onClick).toHaveBeenCalled()
}
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.
still working on case 2...
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.
Done c5ed780
Thanks for the quick code review, give me some more time to update the PR. |
more real use case. Co-authored-by: Yann Braga <yannbf@gmail.com>
Yes, it's ready for review again. |
Thank you so much once again @zhyd1997 ! Sorry it took some time, but I'm getting this in now! |
🚀 PR was released in |
Issue: #47
What Changed
Checklist
Check the ones applicable to your change:
yarn update-all
Change Type
Indicate the type of change your pull request is:
maintenance
documentation
patch
minor
major