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

[no-large-snapshots] Unexpected rules are being enabled for snap files #1559

Open
asapach opened this issue Apr 18, 2024 · 9 comments
Open

Comments

@asapach
Copy link

asapach commented Apr 18, 2024

Since snapshot processor has been removed in #1532 (v28.0), we see that unexpected rules are now being enabled for .snap files from ESLint config, which didn't previously run. Because snap files are generated, it doesn't generally make sense to run the same rule set as the rest of the source code, and disabling such rules via overrides seems like overhead.
It would be good to either have an option to restore the previous behavior where only no-large-snapshots rule was enabled for snapshots, or to minimize the overhead.

Steps to reproduce:

  1. Follow the guide in no-large-snapshots docs
  2. Enable more rules in your ESLint config that might clash with snapshots, e.g. no-irregular-whitespace
  3. See that ESLint now produces more errors than it did prior to v28.0
@G-Rath
Copy link
Collaborator

G-Rath commented Apr 18, 2024

Managing what rules are enabled for particular files should be the responsibility of the configurer, not us, since you can change those in ways that we can't pickup - that's why we don't explicitly configure overrides/files in our shared configs, and why the snapshot processor was removed in the first place (as there are some other rules people did want to apply to their snapshots).

Could you share more details about the setup and config you're using?

@asapach
Copy link
Author

asapach commented Apr 18, 2024

We use eslint:recommended in our root config and jest plugin for our tests, including no-large-snapshots rule. After upgrading to v28.0 we've noticed that some snapshots were causing lint errors, because no-irregular-whitespace was complaining about <NBSP> characters, which as it turns out came from &nbsp; HTML entities in our tests. This was completely unexpected, since no violations were previously reported there. After some investigation we've created an override for *.snap files to modify the no-irregular-whitespace rule: { skipTemplates: true }, which worked around the issue.
However, this presents some questions:

  1. Are more rules being run on snapshots that previously thought?
  2. Was the snapshot processor disabling them or hiding the errors?
  3. How do we make sure that a minimal rule set is enabled for snapshots only? Both for performance and maintenance reasons.

@G-Rath
Copy link
Collaborator

G-Rath commented Apr 18, 2024

I believe this primarily boils down to your configuration - the processor had a postprocess function which filtered out all messages that didn't come from jest/no-large-snapshots, so it would do nothing to prevent rules from being run against snapshot files themselves.

So then it comes down to your config: if you structured your config in a way that meant snapshot files would be targeted by a particular rule then yes it would be run regardless of the processor, and by extension ensuring what rules are applied to snapshots is something for you to manage - off the top of my head I'm pretty sure so long as you're not using --ext snap then all that would be required is to configure jest/no-large-snapshots in a dedicated config block that just targets *.snap, and use ignores in your main Jest config block (which assumably has files: ['path/to/tests/**']) to exclude *.snap.

That might also be the case if you are using --ext snap but I can't remember if that expands the scope of what you've got in your config...? it seems like it would be silly to but I can't say for sure so won't claim that is the be-all-end-all solution :)

The good news is from what I understand about how flat config works this should be a lot simpler since --ext is gone meaning you just need to lay your configs out as I described above and you should be fine.

@sergei-startsev
Copy link

@G-Rath ignores is only available in ESLint 9, and many projects aren't ready to update eslint to the latest version for various reasons (e.g. plugins don't support v9 yet), so it's not the case for most projects at the moment.

@sergei-startsev
Copy link

I'd say it's quite unexpected that eslint recommended rules, which don't make any sense for snapshots, began to apply to them. And the only option to effectively address this issue for many projects is to introduce own preprocessor.

@asapach
Copy link
Author

asapach commented Apr 19, 2024

eslint@8 doesn't have ignores, but it does have ignorePatterns, but the problem is that either way ESLint emits a warning in this case:

warning: File ignored because of a matching ignore pattern. Use "--no-ignore" to override

@G-Rath
Copy link
Collaborator

G-Rath commented Apr 19, 2024

Yeah sorry that's me mixing up the docs - pre v9 it's called excludedFiles

@G-Rath
Copy link
Collaborator

G-Rath commented Apr 19, 2024

Ok so to recap I think there's three main ways you can address this:

  1. switch to configuring all rules via overrides, then you can use excludedFiles to exclude snapshots
    • this shouldn't be that hard (note you just need to move extends and rules - everything else can remain at the top level), and as a bonus is more "flat config"-y
  2. explicitly disable rules that are triggered on snapshot files - so far you've just mentioned the one so it seems pretty easy to just evaluate what to do for rules on snapshots as they come up
  3. use your own processor to enable an "allowlist" of rules to report on snapshots; you only need about 5 lines of code for this

Keep in mind one of the reasons removing the processor came up was because people wanted to run other ESLint rules against their snapshots - so restoring it would only serve your case.

On the note of performance and maintenance: that's on you as the configurer - as a plugin, we have no way to prevent you from running rules on files; the best we can do is filter out messages from rules after they've been run via postprocess.

@asapach
Copy link
Author

asapach commented Apr 19, 2024

I think we'll go with option 1, but it would probably be a good idea to document it in no-large-snapshots docs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants