Feature/rule no dom import

[Belco90]

New rule to force there are no direct imports from @testing-library/dom or
@testing-library/dom when using some testing library framework
wrapper. All info within proper doc file.

[thomaslombart]

Nice job!
There's just the error message that I find unclear. I can't think of anything better than that though 😅

[thomaslombart]

The error message sounds unclear to me. What do you think?

[Belco90]

Yeah, I'm not convinced at all about it but I can't suggest which module should be imported. What I can do is to check which DTL module is used (dom-testing-library or @testing-library/dom) and say something like import directly from '@testing-library/dom' is restricted, instead you must import from '@testing-library/<framework>' by literally leaving that <framework> placeholder

[thomaslombart]

Yes, I would think of something like that too. I see two solutions possible to that:

1. For each rule, we could configure it by adding the selected framework and then use it in the proper rule. For example:

{
  "rules": {
    "testing-library/no-dom-import": ["error", "react"]
  }
}

However, this would require the user to add the framework to each rule where we need to differentiate the frameworks.

2. Generate a set of rules for every corresponding framework in this file. This could become:

module.exports = {
  rules,
  configs: {
    recommended: {
      plugins: ['testing-library'],
      rules: recommendedRules,
    },
    react: {
      plugins: ['testing-library'],
      rules: { ...generateRecommendedRules("react"), 'testing-library/no-debug': 'warn' },
    },
    // ...
  },
}

And in generateRecommendedRules:

function generateRecommendedRules(framework) {
  return allRules.map(rule => {
    const augmentedRule = rule(framework)
    return augmentedRule
  })
}

Where rule would be a higher-order function that returns the proper rule configuration based on the framework:

'use strict';

const { getDocsUrl } = require('../utils');

const DOM_TESTING_LIBRARY_MODULES = [
  'dom-testing-library',
  '@testing-library/dom',
];

module.exports = (framework) => {
  return {
    meta: {
      type: 'problem',
      docs: {
        description: 'Disallow importing from DOM Testing Library',
        // ...
     },
    messages: {
      noDomImport:
        `import from DOM Testing Library is restricted, import from "@testing-library/${framework}`,
    },
    fixable: null,
    schema: [],
  },
  // ...
}

What do you think?

[Belco90]

🤦‍♂️ of course, didn't think about context options! Really interesting approach, I'd go for something intermediate between those 2 options: we add the framework option to this rule through context.options (so you receive it within create function through context arg and pass it to report message, as this seems the standard instead of doing the whole workaround to pass it to the rule function itself) and then we setup automatically this option on each recommended framework config (so it's automatically setup for those frameworks but everyone can customize it when adding the rule manually).

So I'm gonna merge this one (as I wanted to improve the error message but obviously it's gonna be way better when the rule options are received so I'm gonna skip that for now) and create an issue for you to handle that separately, but only for receiving the context.options and improve the message with proper framework setup, nothing yet about fixable. Are you happy with that?

[thomaslombart]

Brilliant!

[thomaslombart]

It would be awesome (and I think pretty easy though I'm not sure of that) to include a fix for this rule. I can look into it if you want.

[Belco90]

It should, and I was thinking about it but I prefer to leave the fixable for a future version. Every time I think about this one it seems more complex to implement as you don't know which TL framework the query must be imported (don't think about react only, this works for vue and angular). So for fixing the rule we could try to guess from where the user should import it by looking for other "@testing-library/" text and get that suffix to replace it by "@testing-library/dom" but then... what if it's the first import in the file referring to testing library? We can't guess from where we have to reimport. Did you have something else in mind for this?

[thomaslombart]

Oh yes that's true. This is the same problem than above, I have two potential solutions 👆