JS-2019: Implement S8985: side effects should not be performed inside "waitFor" callbacks#7488
JS-2019: Implement S8985: side effects should not be performed inside "waitFor" callbacks#7488nathsou wants to merge 6 commits into
Conversation
…" callbacks Import eslint-plugin-testing-library's no-wait-for-side-effects rule, decorated to force strict Testing Library import resolution (the upstream rule's aggressive name-based heuristics would otherwise match any identically-named waitFor/fireEvent/userEvent/render regardless of import origin). Temporarily pins rspec.sha to the RSPEC PR branch tip (SonarSource/rspec#7334) until that PR merges, per docs/BUILD.md.
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
The floating v1 tag now resolves to a commit (8d5f45db5f, "Skip duplicate S3 cache save when content matches the default-branch fallback", merged 2026-06-30) that skips saving the npm cache under a branch's own lockfile-hash key whenever content matches master's, and relies on a fallback restore that doesn't account for a differing lockfile hash - leaving node_modules empty on any branch that changes package-lock.json (like this one, which adds eslint-plugin-testing-library). v1.7.1 predates that change. Pinning here as a test to confirm this is really the regression before deciding between reverting to a pinned version repo-wide vs. the npm-ci-fallback approach in #7494.
Confirmed on PR #7488 that pinning to v1.7.1 (the release before 8d5f45db5f, "Skip duplicate S3 cache save when content matches the default-branch fallback", merged into the gh-action_cache v1 branch on 2026-06-30) resolves the cache-miss/"tsx: not found" failure directly, without needing a fallback install step. That commit is the actual regression: it skips saving an npm cache entry under a branch's own lockfile-hash key whenever content matches master's, relying on a fallback restore that doesn't account for a differing lockfile hash - so restore-only jobs end up with no node_modules at all whenever a branch's lockfile actually differs from master (e.g. a new devDependency, or a Renovate bump). Pinning to v1.7.1 undoes the regression directly instead of working around its effects.
…S8985 Bump the temporary rspec.sha pin from 689ac4b9973 to ec837696ca69, the current tip of SonarSource/rspec#7334 (rule/add-RSPEC-S8985), which adds explicit fix guidance and relevance/false-positive notes to the S8985 rule.adoc. Verified via `npm run rspec:refresh -- -Drspec.sha=<sha>` that S8985.json is unaffected (only the rule description wording changed) and reverted unrelated drift the refresh pulled in for S2068/S6418/S6437.
…kage-lock.json The eslint-plugin-testing-library@7.16.2 entry added for S8985 was generated through the local Artifactory npm mirror (repox.jfrog.io), which differs from the registry.npmjs.org host used by every other lockfile entry (1001 of 1004). Re-resolving through that mirror caused npm to rewrite ~16 unrelated optional @esbuild/* platform packages, dropping their "libc" field. Root cause identified by diffing against sibling commit 9bc5ee5 (S8980), which added the identical dependency+version with a clean, 18-line, zero-churn diff using the registry.npmjs.org host. Fixed by: restoring the 16 stripped "libc" blocks to their exact prior values/ positions (verified byte-identical to the pre-S8985 parent commit b0e0911), and pointing the new package's "resolved" URL at registry.npmjs.org with its unchanged integrity hash (same tarball, confirmed identical sha512 across both mirrors), matching the project-wide convention and S8980's precedent. package-lock.json now diffs against b0e0911 with only the two expected additions: the devDependencies entry and the new node_modules/eslint-plugin-testing-library block, matching S8980's pattern exactly. Verified: JSON is well-formed, no other files touched, and the S8985 unit test still passes.
|
Code Review ✅ Approved 3 resolved / 3 findingsImplements rule S8985 to restrict side effects in waitFor callbacks and refines upstream rule resolution to prevent false positives. Reverted temporary CI cache and RSPEC pinning configurations as requested. ✅ 3 resolved✅ Quality: Temporary CI cache-action pin should be reverted before merge
✅ Quality: rspec.sha pinned to a temporary RSPEC PR branch tip
✅ Edge Case: Forcing utils-module off may cause false negatives for custom re-exports
OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
1 participant
Summary
no-wait-for-side-effectsrule.testing-library/utils-module,custom-renders,custom-queriesset tooff), since the upstream rule's aggressive name-based heuristics would otherwise match any identically-namedwaitFor/fireEvent/userEvent/renderregardless of where it's actually imported from.waitFor(compliant), side effect inside a.then()chained afterwaitFor()(compliant), same-named non-Testing-LibrarywaitFor/renderhelpers (compliant), block-statement and implicit-return side effects insidewaitFor(non-compliant, with quickfix), and aliased Testing Library imports (non-compliant).rspec.shato the RSPEC PR branch tip until that PR merges, perdocs/BUILD.md.Links
Summary by Gitar
gh-action_cachetov1.7.1across all CI workflows to address cache-miss issues.testing-libraryexternal plugin and documented theS8985rule mapping.S8985in the defaultSonar wayquality profile.This will update automatically on new commits.