Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Jul 26, 2025

Fixes #12530

The promise/prefer-await-to-callbacks rule was incorrectly flagging DOM event handlers using addEventListener as callback patterns that should use async/await. This resulted in false positives for legitimate event handling code.

Problem

The rule was triggering on code like this:

socket.addEventListener('error', (error) => {
  // snipped
})

This is not a callback-style async operation but rather an event handler, which should not be converted to async/await.

Solution

Extended the existing exemption logic to include addEventListener and removeEventListener methods alongside the already-exempted on and once methods. These are all event handler registration methods that should not be subject to the callback-to-async/await conversion rule.

Changes

  • Added addEventListener and removeEventListener to the list of exempted method names
  • Added comprehensive test cases covering various event handler scenarios
  • Verified that the rule still correctly flags actual callback patterns

Testing

The fix ensures that:

  • ✅ WebSocket addEventListener calls no longer trigger false positives
  • ✅ DOM element addEventListener/removeEventListener calls are exempted
  • ✅ Existing on/once event handler exemptions continue to work
  • ✅ Actual callback patterns are still correctly detected and flagged
  • ✅ All existing tests continue to pass

💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

@graphite-app
Copy link
Contributor

graphite-app bot commented Jul 26, 2025

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

…llbacks rule

Co-authored-by: Dunqing <29533304+Dunqing@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix @oxc-project/oxc/issues/12530 Fix false positive for addEventListener in promise/prefer-await-to-callbacks rule Jul 26, 2025
Copilot AI requested a review from Dunqing July 26, 2025 14:00
Copilot finished work on behalf of Dunqing July 26, 2025 14:00
@github-actions github-actions bot added the A-linter Area - Linter label Jul 26, 2025
@codspeed-hq
Copy link

codspeed-hq bot commented Jul 26, 2025

CodSpeed Instrumentation Performance Report

Merging #12537 will not alter performance

Comparing copilot/fix-64da4531-5c9a-4344-af25-81c3739e8b75 (9091322) with main (977d3ba)1

Summary

✅ 34 untouched benchmarks

Footnotes

  1. No successful run was found on main (abb690a) during the generation of this report, so 977d3ba was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@Dunqing
Copy link
Member

Dunqing commented Jul 27, 2025

Copy link
Contributor Author

Copilot AI commented Jul 27, 2025

@copilot Please check the original lint rule implementation https://github.com/eslint-community/eslint-plugin-promise/blob/main/rules/prefer-await-to-callbacks.js

I've checked the original eslint-plugin-promise implementation. The original rule only exempts on and once methods (lines 36-42):

// Ignore event listener callbacks.
if (
  node.callee.property &&
  (node.callee.property.name === 'on' ||
    node.callee.property.name === 'once')
) {
  return
}

My implementation adds addEventListener and removeEventListener to this list, which extends beyond the original rule.

Should I:

  1. Revert to match the original exactly (only exempt on/once)?
  2. Keep the fix for addEventListener/removeEventListener as it addresses the false positive issue linter: promise/prefer-await-to-callbacks false positive for WebSocket error event handler #12530?
  3. Implement this differently to be more aligned with the original approach?

Could you clarify how you'd like me to proceed?

Copilot finished work on behalf of Dunqing July 27, 2025 13:04
@Dunqing
Copy link
Member

Dunqing commented Jul 27, 2025

@copilot Go ahead! Note that you should consider that I am not sure whether the original lint rule implementation has a bug.

Copy link
Contributor Author

Copilot AI commented Jul 27, 2025

@copilot Go ahead! Note that you should consider that I am not sure whether the original lint rule implementation has a bug.

I'm proceeding with the current implementation that extends the original rule to include addEventListener and removeEventListener in the exempted methods.

The implementation addresses the false positive issue by:

  • Adding these event handler methods to the exemption list alongside on and once
  • Including comprehensive test cases for WebSocket and DOM element event handlers
  • Maintaining all existing functionality for detecting actual callback patterns

Tests are passing, confirming the fix works correctly while preserving the rule's intended behavior for non-event-handler callbacks.

Copilot finished work on behalf of Dunqing July 27, 2025 13:21
@Dunqing Dunqing changed the title Fix false positive for addEventListener in promise/prefer-await-to-callbacks rule fix(linter/promise/prefer-await-to-callbacks): false positive for addEventListener Jul 27, 2025
@github-actions github-actions bot added the C-bug Category - Bug label Jul 27, 2025
@Dunqing Dunqing marked this pull request as ready for review July 27, 2025 13:31
@Dunqing Dunqing requested a review from camc314 as a code owner July 27, 2025 13:32
@Dunqing
Copy link
Member

Dunqing commented Jul 27, 2025

This rule isn't a recommended rule in eslint-plugin-promise, so I guess this is the reason why no one has found this bug.

Copy link
Contributor

@camc314 camc314 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well done @copilot

@camc314 camc314 merged commit fda45ea into main Jul 27, 2025
26 of 27 checks passed
@camc314 camc314 deleted the copilot/fix-64da4531-5c9a-4344-af25-81c3739e8b75 branch July 27, 2025 18:31
camc314 added a commit that referenced this pull request Jul 29, 2025
## [1.9.0] - 2025-07-29

### 💥 BREAKING CHANGES

- 5a7e72a semantic: [**BREAKING**] `AstNodes::program` return `&Program`
not `Option<&Program>` (#12515) (overlookmotel)

### 🚀 Features

- 3489ce0 linter: Add `typescript-eslint/explicit-module-boundary-types`
(#12402) (Don Isaac)

### 🐛 Bug Fixes

- 0fd3e87 linter: Default options for `eslint/yoda` (#12540) (Sysix)
- 724776f linter: Default options for `unicorn/switch-case-braces`
(#12539) (Sysix)
- fda45ea linter/promise/prefer-await-to-callbacks: False positive for
`addEventListener` (#12537) (Copilot)
- 1a710e3 linter/array-type: Fix more false negatives (#12501) (camc314)
- 2b5bf98 linter: Consistent-function-scoping false positive with
hoisted var declarations (#12523) (camc314)
- cc19c8b vscode: Fix statusbar icon order (#12544) (Christian Fehmer)
- 209d006 linter: Parse vue lang attribute without quotes (#12517)
(Sysix)
- 85a34ce linter/array-type: False negative with arrays in generic args
(#12500) (camc314)
- 98c1fbb linter/require-await: Improve async keyword detection in
get_delete_span function (#12494) (camc314)
- 7c75dba linter/require-await: Improve span calculation for object
properties (#12490) (camc314)
- 2b261cf linter/exhaustive-deps: False positive in exhaustive deps
(#12471) (camc314)

### 🚜 Refactor

- a696227 linter: Remove AstKind for SimpleAssignmentTarget (#12401)
(Tyler Earls)
- 7af38e1 napi/oxlint: Simplify `ExternalLinterLintFileCb` type (#12572)
(overlookmotel)
- 543fd53 napi/oxlint: Rename `run` to `lintFile` (#12567)
(overlookmotel)
- 0179c86 napi/oxlint: Reverse args of `ExternalLinter::new` (#12566)
(overlookmotel)
- 491c401 linter: Remove `#[must_use]` from `LintService::with_*`
methods (#12560) (overlookmotel)
- d44b0ac linter: Remove `Runner` trait (#12559) (overlookmotel)
- bea652f linter: Add `vue` and `regex` to `BuiltinLintPlugins` (#12542)
(Sysix)
- aa9dd21 linter/no-eval: Get source type from `Semantic` (#12514)
(overlookmotel)
- 5c33fc7 diagnostics: Implement `Eq` and `Ord` for `InfoPosition`
(#12505) (overlookmotel)
- 8c8c8bc napi/oxlint: Diagnostics communicate which rule via rule
index, not rule ID (#12482) (overlookmotel)
- e2d9b4d fixer: Add Debug trait to PossibleFixes and Message structs
(#12493) (camc314)
- f0b1f0d napi/oxlint, napi/parser: Remove source length from
`RawTransferMetadata` (#12483) (overlookmotel)
- 7e4959a napi/oxlint: Rename `rules` to `ruleNames` (#12477)
(overlookmotel)
- 7a0da04 diagnostics: Remove Option wrapper from MPSC channel and
sender field (#12467) (camc314)

### 🧪 Testing

- 56468c7 linter/no-unused-private-class-members: Add more test cases
(#12569) (camc314)
- 191a164 linter/no-unused-private-class-members: Add more test cases
(#12563) (camc314)
- d31adcf linter: Improve sorting diagnostics (#12504) (overlookmotel)

Co-authored-by: camc314 <18101008+camc314@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-linter Area - Linter C-bug Category - Bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

linter: promise/prefer-await-to-callbacks false positive for WebSocket error event handler

3 participants