Dom-testing-library: eslint plugin

[Belco90]

Describe the feature you'd like:

An eslint plugin for testing library, specially for DOM testing library queries, similar to eslint-plugin-jest. I usually find myself or some colleagues writing findBy* queries without proper await or writing non-async queries with await. I would find really helpful to get some eslint warnings about where I missed or put unnecessary await. There are probably more interesting things we could add to this plugin in the future, but I think for a first approach this is enough.

I don't know if this is the best place to open the issue because it would imply to create a different repo for it but, as this is the package I'm more interested into getting the eslint plugin for, I finally created the issue here.

Suggested implementation:

Never implemented an eslint plugin before or an eslint rule either, but I'm happy to do it first time now.

Describe alternatives you've considered:

I've tried installing dom-testing-library and react-testing-library types but didn't help me to get those warnings I want.

Teachability, Documentation, Adoption, Migration Strategy:

This would be an additional package that users can install independently and config for eslint, so would need to setup its own repo and package (I would need some help with the setup probably). My idea is to just add 2 rules for the cases mentioned in the issue description (including a recommended config that enables them as warnings), and then keep working on more rules and even autofixing.

[kentcdodds]

That sounds amazing! Yes, let's do this. Here's how I think it should happen. Go ahead and start work on it in your own new repo. Once you have something ready then we can talk about bringing it into the org. 🤘

[thomlom]

Hey @Belco90, I'm interested in doing a plugin for testing-library too. Do you need help with this? 🙂

[Belco90]

@kentcdodds nice, that makes sense to me! I'm gonna be little bit busy next few weeks and it's my first time working with eslint plugin so it will take a while for me to get something ready, but I'll keep you updated about it.

@thomlom I'll need help for sure, thank you! Let me create the repo and make a basic setup so you can help me with it. Anything in particular you would like to work on or you feel more comfortable?

[thomlom]

@Belco90 I've written a few ESLint rules before but no plugins. Feel free to setup it as you want, we'll continue discussing there 🙂

[Belco90]

@thomlom nice! I'm creating the plugin setup with a first approach for those rules and then I'll share it here with you so we can discuss best implementation for them. I'm really impressed about how easy is to make the plugin and generate new rules for eslint with their scripts!

[Belco90]

@thomlom hey there! Doing some progress here, but I'm struggling to identify some edge cases.
For no-await-sync-queries I don't have any issues, as this one is much easier to implement (it's not pushed to my repo as I'm still adding proper docs and tests). But await-async-queries rule is killing me. Here you can find my progress so far: https://github.com/Belco90/eslint-plugin-testing-library/blob/master/lib/rules/await-async-queries.js (and proper tests under tests folder)

Basically my problem now is I don't know how to assume that if you return the promise from find* queries this is a valid case (I got a failing test for it). Also I'm not convinced at all about the first implementation to know if the find* query as proper await as I'm just looking for parent of its parent and checking if it's an await type.

Let me know what you think about all this, I'll keep working on the other rule meanwhile!

[Belco90]

@kentcdodds Just FYI: I got a first version of the plugin working!

I've installed it in a huge project from the company I work for and the rule no-await-sync-query seems to be working fine already (this one is easy to implement), but await-async-query one needs more work as it's giving some false positives and doesn't cover some edge cases yet. I'll be adding more tests and doc this week, and trying to improve the rule for async queries until @thomlom is available so he can review my progress so far!

[kentcdodds]

That's great! Note that the await-async-query rule should probably mention that a return or a .then is acceptable as well.

return findByText(/blah/i) // returns the promise which is fine

findByText(/blah/i).then(node => {
  // stuff
  done()
})

Kinda non-conventional, but still should be supported.

[Belco90]

Exactly! Those are the edge cases I still need to cover. Checking the promise is returned within a function or has a chained .then is not that difficult and I'm working on that, but it can be also saved in a var so I need to track properly what's happening with that promise. I don't think saving the promise returned by findBy* is that common so I'll try to cover the return (for both regular and arrow functions) and the chained .then and see if we could leave the promise var tracking for a future improvement.

[afontcu]

Hi! First of all, I wanna say that having an eslint plugin is an awesome idea 👍

I was wondering how extensible a ruleset is? For instance, Vue Testing Library has some particular rules which could be enforced by a linter. Specifically, both fireEvent and fireEvent[event] are async, so same findByX rules apply (await fireEvent.click(node)).

So here's an idea: could we have rules such as testing-library/vue-no-await-sync-fireevent (turned off by default) so Vue users could enable it manually? Is there a better solution? (Haven't written an eslint plugin before, so it might!)

Thanks! 🙌

[Belco90]

Whoa, didn't know that fireEvent was async on Vue! It makes sense to add a new rule await-fire-event or something similar but I would try to leave the rules framework-agnostic.

What we could do to combine those agnostic rules with different frameworks is 1) explicitly mention that the rule is expected to be applied only for some specific frameworks on rule doc file (and maybe even add a column to the available rules table indicating to which framework applies) and 2) add shareable configuration for each framework (similar to style on eslint-jest) so you only had to include "plugin:testing-library/vue" in your eslint extends, which would enable recommended + specific vue rules, and additionally we could have "plugin:testing-library/react" and so on, so anyone can enable/disable sets of rules in particular or by framework. How does that sound?

[afontcu]

Having something like plugin:testing-library/vue feels like the right solution, and in sync with the extension strategy from eslint 👌

No need to implement that right away, just wanted to make sure that we are choosing a path that doesn't block us in a near future :)

[weyert]

Amazing work @Belco90 :D

[Belco90]

Hey guys! Quick update about the state of this plugin: we are really close to be ready for v1, feel free to check the rules and configs we have so far: https://github.com/Belco90/eslint-plugin-testing-library

You can give the plugin a try installing it directly from github like: npm i -D git+ssh://[email protected]:Belco90/eslint-plugin-testing-library.git and then configuring proper things on your .eslintrc

Thanks a lot to @thomlom for his work: he's a collaborator of the repo and has been dealing with some edge cases, project setup and no-debug rule! Fantastic work, really appreciate it.

I would say the pending things for v1 are:

• improving how errors for await-sync-query are reported. With current version you'll get several errors for same query not awaited every time you use the var where you kept the matched element. This code will raise 2 errors for example:

const button = findByText('submit');
expect(button).toHaveClass('foo');
fireEvent.click(button);

• add await-fire-event rule and include it on vue config. I think it's a useful rule and should be similar to await-sync-query for being implemented

@kentcdodds what should be next step for this?

[kentcdodds]

This is terrific progress! I tried it out on the examples repo and got no warnings which is a good sign 😅

Sounds like you're ready to run on this one! I think that conventional wisdom is to not use a scoped package for eslint plugins (at least, babel doesn't do it that way), so you can feel free to publish it as eslint-plugin-testing-library and we can add documentation to testing-library.com for it. You can feel free to keep it on your own user account, or we can add it to the github org. Let me know what you'd like to do :)

[kentcdodds]

Whoops, didn't mean to close this.

[Belco90]

@kentcdodds well they mention scoped package as a possibility on their doc:

Each plugin is an npm module with a name in the format of eslint-plugin-<plugin-name>, such as eslint-plugin-jquery. You can also use scoped packages in the format of @<scope>/eslint-plugin-<plugin-name> such as @jquery/eslint-plugin-jquery or even @<scope>/eslint-plugin such as @jquery/eslint-plugin.

So we could publish it as @testing-library/eslint-plugin but to be honest I've never seen any eslint plugin published under scoped packages, so I'm happy with either eslint-plugin-testing-library or scoped one.

I'd like to keep it in my account for now until the library is more mature at least. I'll let you know when we finish those couple of pending things!

Thanks.

[kentcdodds]

That all sounds great to me 👍

[kentcdodds]

Actually, that being the case, I think we can close this issue. I look forward to a pull request to the docs :)

[Belco90]

eslint-plugin-testing-library v1 released! https://www.npmjs.com/package/eslint-plugin-testing-library

I'll create the PR for the docs soon.