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

fireEvent with Promises #166

Closed
timdeschryver opened this issue Jun 16, 2020 · 6 comments · Fixed by #180
Closed

fireEvent with Promises #166

timdeschryver opened this issue Jun 16, 2020 · 6 comments · Fixed by #180
Assignees
Labels
new rule New rule to be included in the plugin released on @beta released
Milestone

Comments

@timdeschryver
Copy link
Member

Based on testing-library/dom-testing-library#609, I think we could add a new rule. I'm not 100% sure if it's worth it, because:

  • testing library already added a runtime error
  • in TypeScript projects, this will already be caught (I think)
  • the current implementation of the rule doesn't catch all cases, and I'm not sure if it can. Currently it catches fireEvent(Promise), fireEevent(new Promise(), fireEvent(findBy...)

The implementation of the rule might look like this.
What do you tink?

export default ESLintUtils.RuleCreator(getDocsUrl)<Options, MessageIds>({
  name: RULE_NAME,
  meta: {
    type: 'suggestion',
    docs: {
      description: 'TK',
      category: 'Best Practices',
      recommended: false,
    },
    messages: {
      noPromiseInEvent: 'TK',
    },

    fixable: 'code',
    schema: [],
  },
  defaultOptions: [],

  create(context) {
    return {
      'ImportDeclaration[source.value=/testing-library/]'(
        node: TSESTree.ImportDeclaration
      ) {
        const fireEventImportNode = node.specifiers.find(
          specifier =>
            isImportSpecifier(specifier) &&
            specifier.imported &&
            'fireEvent' === specifier.imported.name
        ) as TSESTree.ImportSpecifier;

        const {references} = context.getDeclaredVariables(fireEventImportNode)[0];

        for (const reference of references) {
          const referenceNode = reference.identifier;
          if (
            isMemberExpression(referenceNode.parent) &&
            isCallExpression(referenceNode.parent.parent)
          ) {
            const [element] = referenceNode.parent.parent
              .arguments as TSESTree.Node[];
            if (element) {
              if (isCallExpression(element) || isNewExpression(element)) {
                const methodName = isIdentifier(element.callee)
                  ? element.callee.name
                  : isMemberExpression(element.callee) &&
                    isIdentifier(element.callee.property)
                  ? element.callee.property.name
                  : '';

                if (
                  ASYNC_QUERIES_VARIANTS.includes(methodName) ||
                  methodName === 'Promise'
                ) {
                  context.report({
                    node: element,
                    messageId: 'noPromiseInEvent',
                  });
                }
              }
            }
          }
        }
      },
    };
  },
});
@timdeschryver timdeschryver added the new rule New rule to be included in the plugin label Jun 16, 2020
@Belco90
Copy link
Member

Belco90 commented Jun 16, 2020

Oh wow, I didn't even know about this error and I just realized I suffered it!

It could be a good addition to help users spotting the issue in this particular case, though having already a runtime error from testing library seems enough, doesn't it? Not sure what we should do, but you almost got the implementation 😅

@gndelia
Copy link
Collaborator

gndelia commented Jun 16, 2020

in TypeScript projects, this will already be caught (I think)

confirming that ☝️

image

The library (in runtime) and TS (in compile time) also offer a good coverage against the scenario, but given the implementation is almost done, I'd say let's go with it 😄

@timdeschryver
Copy link
Member Author

👍 I'll add it later this week with and add test cases/docs, we can still decide if we want to add it then 🙂
Thank god I added the code here.... since I just reverted by changes locally 😂

@Belco90
Copy link
Member

Belco90 commented Jun 18, 2020

Sorry I had to do it
image

@github-actions
Copy link

🎉 This issue has been resolved in version 4.0.0-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions
Copy link

🎉 This issue has been resolved in version 4.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new rule New rule to be included in the plugin released on @beta released
Projects
None yet
3 participants