[await-async-query] Support custom queries and wrappers

[CreativeTechGuy]

As documented here, you can create custom queries which extend the functionality of the basic query types. The rules of the eslint plugin should apply the same rules to these custom queries as they also apply.

I've identified the two lines that are the root cause of this issue:

• This line causes the await-async-query rule to be hard-coded to only specifically named queries and thus won't support any custom ones.
• This line specifies the only libraries which are valid to import from. This prevents you to wrap testing-library and re-export it's functionality under a new name (as documented here).

I'm happy to submit a PR to fix this but I don't know how to best approach it.

• Should these restrictions be more laxed so that anything which matches the general pattern is included? This would be easier on the developer because there's less to configure and it "just works" but could have some false positives.
• Or should an additional configuration option be added to specify the names of imports and queries which should be added to the rule? This would be more flexible but require the developer to manually configure it whenever they add/change something.

I'm inclined to go with the first option as generally you want your tooling to be smart enough to just work. Any thoughts on this approach or any other recommended approaches?

Oh and let me know if this problem affects any other rules that I haven't run into.

[CreativeTechGuy]

Found a few more occurrences of this issue:

• no-await-sync-query
• import sources affect many other (maybe all) rules so this should probably be a top level option.
• no-manual-cleanup has a separate hard-coded list of sources
• ALL_QUERIES_METHODS should also have customization because that's used in several places such as prefer-explicit-assert
• prefer-presence-queries also has it's own unique way of building the list, but is building it partially based on the utils.

The change needs to be done at the top level utils config and the rules need to be made more generic so they match patterns rather than specific names of imports and queries and any rules which have their own custom list building logic should be refactored to depend on the utils for consistency.

[Belco90]

Hey @CreativeTechGuy, excellent analysis!

Indeed, we should include custom queries to be reported under certain rules. It would be nice to go for your option one, but from previous experience it's too much guessing. How would you know it's a custom query? Just because it starts by getBy, queryBy or findBy? Technically the user could add something custom starting by that which is not a query, or add queries which don't start by those prefix. On the other hand, Testing Library queries should follow that pattern, but just saying. What if a getBy method is found but it's not actually related to Testing Library, but we consider it as custom query?

I prefer option 2 so the user can explicitly set it up as in prefer-explicit-assert. The problem is we are doing this at rule level. We are changing the way the plugin is setu up in next v4. So the idea is to have a shared config for the whole plugin where you add custom render methods you want to consider as part of Testing Library, and also custom module in case you are reexporting Testing Library utils from your custom module by any reason. Then there would be some new utils to be used in the rules implementation to be able to find utils methods from both Testing Library default or custom user code.

Should we included a 3rd option here for custom queries to be taken into account? I think it could be a good idea, but I don't know if we are asking the user to setup too many things. Is there any better way or approach to do this? I'm the one dealing with that task for updating the config, and I'm kind of blocked at the moment so there is no problem on changing the whole approach for all this. I'd like to know your opinion on this after reading that config change!

[CreativeTechGuy]

My opinion is that if the user is writing their own custom queries or wrapping and re-exporting the library, they've opted themselves into "advanced mode" which should be expected to come with additional configuration. Sure it'd be nice for the plugin to magically work, but I'd rather have to configure a few extra things than dig through the plugin's source code for several hours to find how to update it and file an Issue on GitHub! 😛

The concern I have is that it's very hard to debug eslint rules in general. If you don't have any code which violates a rule and especially if you don't know/remember the rules that were configured, you won't know there was anything wrong and anything you need to fix. You could silently have opted-out of this plugin just by wrapping and re-exporting the library and not know you need to do something extra to configure it.

For that reason, I think the solution is actually in two parts. The plugin should be overly aggressive by default. It should match any method which is used in a test that starts with any of the standard prefixes and it shouldn't care where it was exported from. For many users this won't create false positives and will just work. If this does create a problem they can configure the global config to opt-out of this aggressive matching and/or specify the specific imports and methods which to use. This way a user wouldn't end up in a state where they disabled the plugin without realizing it.

I think in general, my preferred configuration is one that has several levels. There's the base case that works for most users and comes out of the box. There's the slight tinkering which involves a few basic options being turned on or off. And finally there's the advanced user who knows exactly what they are doing and exactly what they want and they should be able to configure almost everything if they have a need to.

To me it's very frustrating when I like most of what a library offers but there's just a few lines of code that I wish were changed to make it perfect for my situation. Luckily patch-package exists for those cases, but it'd be better if there were more options that the user could change.

[Belco90]

Fair enough, really interesting point of view. I like the idea about the plugin should be aggressive to avoid opting-out some rule by doing an advanced configuration. I need to think about it and repurpose v4 if we decide to go in this direction, as v4 was planned to go in the opposite way: just check whatever is imported from testing library by default.

Sorry for involving you in all this v4 thing, but the issue you raised here is really attached to the changes we were planning to do.

[Belco90]

@CreativeTechGuy Hi again! I just wanted to let you know that your explanation made me change my mind for v4 of the plugin so it's been refactored to be more aggressive + have more granular control over scope of the plugin. You can check this in this PR for example.

When the v4 is released, the original problems mentioned here will be solved. Thanks for your feedback, it was really helpful!

[CreativeTechGuy]

@Belco90 I've been keeping up with your progress and I'm very excited. I've had to maintain a modified version of the library to support this for my codebase and I'll be excited to remove that when this is released.

I have zero experience with ESLint plugin development but do let me know if there's some way I can help or provide feedback. :)

[Belco90]

Thanks for your help! I think I'm fine for now, but probably would nice if you could test the beta of v4 when released.

[Belco90]

This should be fixed on v4.0.0

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

[9still]

@CreativeTechGuy @Belco90 I realize that this has already been done, but I want to vote against enabling the aggressive mode by default precisely for the reason of eslint config being hard to debug. I would argue that although I have no way to prove this, I suspect that the vast majority of users will NOT have the types of patterns that you listed as rationale for enabling the aggressive behavior.

As implemented, the rules end up creating tons of false positives, and flagging stuff that isn't even in test files & has nothing to do with testing library. It feels really dangerous to assume that testing-library has an exclusive on certain function name patterns, especially something as common as getById/getByIds, since these are very commonly used in application code.

By opting people in to this behavior by default you're causing a bunch of unnecessary work for a ton of plugin users that will provide 0 benefit to them. I think providing good documentation for advanced users that want to opt into this behavior for specific utils would be a considerably better solution.

Also see #331