Skip to content

fix(core): invoke structured Signal listeners with runtime args before bound args#2987

Open
cptbtptpbcptdtptp wants to merge 2 commits into
galacean:dev/2.0from
cptbtptpbcptdtptp:pr/signal-arg-order
Open

fix(core): invoke structured Signal listeners with runtime args before bound args#2987
cptbtptpbcptdtptp wants to merge 2 commits into
galacean:dev/2.0from
cptbtptpbcptdtptp:pr/signal-arg-order

Conversation

@cptbtptpbcptdtptp
Copy link
Copy Markdown
Collaborator

@cptbtptpbcptdtptp cptbtptpbcptdtptp commented May 11, 2026

Summary

  • Flip structured-binding listener invocation order from method(...boundArgs, ...signalArgs) to method(...signalArgs, ...boundArgs), so the event object always sits at index 0.
  • Aligns with DOM (event, customData) convention.

Background

Cherry-picked from b16d8b5d7 on fix/shaderlab.

Test plan

  • 3 new Signal.test.ts cases: arg ordering, event index 0 invariant, once() parity
  • All 29 Signal tests pass
  • CI green

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Corrected Signal structured binding behavior: runtime arguments now properly precede bound arguments during method invocation. The event object remains at index 0, restoring expected argument ordering for handlers using bound arguments.
  • Tests

    • Added comprehensive test coverage for Signal structured binding, validating correct argument ordering with bound arguments and ensuring single-use listeners maintain expected behavior.

Review Change Stack

cptbtptpbcptdtptp and others added 2 commits May 11, 2026 18:01
…e bound args

Previously `Signal._addListener` applied bound arguments first and runtime
signal args last, producing `method(...boundArgs, ...signalArgs)`. This made
the event object's position shift with the number of bound arguments and
diverged from the DOM / Cocos `(event, customEventData)` convention.

Flip the order so the listener method receives runtime args first and bound
args last: `method(...signalArgs, ...boundArgs)`. The event object now always
sits at index 0, making migrated Cocos scripts with `(event, customData)`
signatures work without rewrites.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add three test cases verifying that runtime signal args are passed
before bound args:

- "runtime args precede bound args" — full ordering with two of each.
- "event object stays at index 0 regardless of bound args count" —
  position invariant of the conventional event argument.
- "once: runtime + bound args order preserved" — same guarantee for
  once-style listeners.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 11, 2026

Walkthrough

Signal's structured binding listener changes argument order: runtime signal arguments now precede pre-resolved bound arguments when invoking target methods. Implementation, documentation, and test coverage are updated to reflect and validate this new invocation contract.

Changes

Signal Argument Ordering for Structured Binding

Layer / File(s) Summary
Core Implementation
packages/core/src/Signal.ts
Listener wrapper for structured binding now calls target method as target[methodName](...signalArgs, ...args) instead of target[methodName](...args, ...signalArgs).
Documentation
packages/core/src/Signal.ts
JSDoc for both on and once structured binding overloads now document that runtime signal arguments are passed first and bound arguments are appended.
Test Infrastructure
tests/src/core/Signal.test.ts
TestHandler gains lastArgs field to record received arguments and handleWithArgs(...args) method to support argument ordering validation.
Test Validation
tests/src/core/Signal.test.ts
New test cases verify runtime args precede bound args, event object stays at index 0 regardless of bound-arg count, and once preserves argument order for both invocation count and captured arguments.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 Arguments dancing in a line,
Runtime first, then bound ones shine!
Signal whispers, listeners hear,
Order matters, crystal clear! ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: fixing structured Signal listeners to invoke with runtime arguments before bound arguments, which aligns with the documentation and code changes in both Signal.ts and the test file.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

GuoLei1990

This comment was marked as outdated.

GuoLei1990

This comment was marked as outdated.

GuoLei1990

This comment was marked as outdated.

@GuoLei1990 GuoLei1990 mentioned this pull request May 11, 2026
3 tasks
GuoLei1990

This comment was marked as outdated.

GuoLei1990

This comment was marked as outdated.

Copy link
Copy Markdown
Member

@GuoLei1990 GuoLei1990 left a comment

Choose a reason for hiding this comment

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

总结

将 Signal 结构化绑定监听器的调用顺序从 method(...boundArgs, ...signalArgs) 修正为 method(...signalArgs, ...boundArgs),确保 event 对象始终在 index 0,与 DOM addEventListener 约定对齐。

一行改动,三个新测试覆盖 arg ordering、event at index 0 不变量、once() 行为一致性。

无新问题,LGTM。

Copy link
Copy Markdown
Member

@GuoLei1990 GuoLei1990 left a comment

Choose a reason for hiding this comment

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

总结

将 Signal 结构化绑定监听器的调用顺序从 method(...boundArgs, ...signalArgs) 修正为 method(...signalArgs, ...boundArgs),使事件对象始终在参数列表第 0 位,与 DOM 约定对齐。测试覆盖了顺序验证、事件对象位置不变性、once() 对等性。

修复方向正确,实现最小化。无新问题,可以合并。

Copy link
Copy Markdown
Member

@GuoLei1990 GuoLei1990 left a comment

Choose a reason for hiding this comment

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

总结

将 structured binding 的参数顺序从 (...args, ...signalArgs) 改为 (...signalArgs, ...args),与 DOM (event, customData) 惯例对齐。修复正确,3 个新测试验证了参数顺序不变量。

问题

[P2] Signal.ts:175 — 更改前代码把 bound args 放前面,这意味着已有用 structured binding 并依赖旧顺序的代码(method(boundArg, event))会 silently break。PR 描述里没有 migration guide。建议在 CHANGELOG 或 Breaking Changes 里明确标注旧语义。

无阻塞问题,LGTM。

Copy link
Copy Markdown
Member

@GuoLei1990 GuoLei1990 left a comment

Choose a reason for hiding this comment

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

总结

逻辑修复正确,与 DOM addEventListener(event, customData) 惯例对齐,测试覆盖完整(arg ordering + event index 0 invariant + once parity)。

问题

[P1] Breaking change 需在 CHANGELOG 中标注

使用 structured binding 且传了 bound args 的现有代码(signal.on(component, 'handler', boundArg1)),升级后 handler 收到的参数顺序从 (boundArg1, ...signalArgs) 变为 (...signalArgs, boundArg1)dev/2.0 允许 breaking changes,但需在 changelog 或 PR 描述中明确标注:旧顺序 (bound, ...runtime) → 新顺序 (...runtime, bound),以便用户迁移。

Copy link
Copy Markdown
Member

@GuoLei1990 GuoLei1990 left a comment

Choose a reason for hiding this comment

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

总结

将 Signal 结构化绑定监听器的调用顺序从 method(...boundArgs, ...signalArgs) 修正为 method(...signalArgs, ...boundArgs),确保 event 对象始终位于 index 0,与 DOM addEventListener (event, customData) 约定对齐。

改动一行,方向完全正确,三个新测试覆盖了参数顺序、event-at-index-0 不变量、once() 行为。JSDoc 也同步更新了参数顺序描述。

问题

无新问题。LGTM,可合入。

(注:这是 dev/2.0 范围内的 breaking change,使用 structured binding 并传了 bound args 的现有代码需适配新顺序。建议在 release notes 中标注旧顺序 (bound, ...runtime) → 新顺序 (...runtime, bound)。)

Copy link
Copy Markdown
Member

@GuoLei1990 GuoLei1990 left a comment

Choose a reason for hiding this comment

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

已关闭问题清单

所有历史问题均已关闭。

总结

将 Signal 结构化绑定监听器的调用顺序从 method(...boundArgs, ...signalArgs) 修正为 method(...signalArgs, ...boundArgs),确保 event 对象始终在 index 0,与 DOM addEventListener约定对齐。改动一行,方向完全正确,三个新测试验证了参数顺序、事件对象位置不变性(含多 bound args 场景)和 once() 对等性。

[P2] 破坏性变更建议记录在 CHANGELOG

...boundArgs, ...signalArgs...signalArgs, ...boundArgs 是破坏性变更:已有使用结构化绑定且依赖旧参数顺序的用户代码会静默接收到错误参数而不报错。若项目维护 CHANGELOG,建议在 2.0 breaking changes 中加一条说明。不阻塞合并,但有助于下游感知。

其余无新问题。LGTM。

Copy link
Copy Markdown
Member

@GuoLei1990 GuoLei1990 left a comment

Choose a reason for hiding this comment

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

已关闭问题清单

所有历史问题均已关闭。

总结

将 Signal 结构化绑定监听器的调用顺序从 method(...boundArgs, ...signalArgs) 修正为 method(...signalArgs, ...boundArgs),确保 event 对象始终在 index 0,与 DOM addEventListener 约定对齐。改动一行,方向完全正确,三个新测试验证了参数顺序、事件对象位置不变性(含多 bound args 场景)和 once() 对等性。

问题

无新问题。

[P2] 这是破坏性变更(...boundArgs, ...signalArgs...signalArgs, ...boundArgs),已有使用结构化绑定且依赖旧参数顺序的用户代码会静默收到错误参数而不报错。建议在 2.0 breaking changes CHANGELOG 中加一条说明。不阻塞合并。

LGTM,可合入。

Copy link
Copy Markdown
Member

@GuoLei1990 GuoLei1990 left a comment

Choose a reason for hiding this comment

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

审查(2026-05-15)

总结

修复 Signal 结构化绑定监听器的参数顺序——从 method(...boundArgs, ...signalArgs) 改为 method(...signalArgs, ...boundArgs),使 event 对象始终在 index 0。与 DOM 约定 (event, customData) 对齐.

测试覆盖三种场景:参数顺序、event 在 index 0 的不变量、once() 的一致性。

整体 LGTM,可以合入。

Copy link
Copy Markdown
Member

@GuoLei1990 GuoLei1990 left a comment

Choose a reason for hiding this comment

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

总结

将结构化绑定 listener 的调用顺序从 method(...boundArgs, ...signalArgs) 修复为 method(...signalArgs, ...boundArgs),使 event 对象始终在第一个参数位置,与 DOM (event, customData) 惯例对齐。

修复正确,测试覆盖了参数顺序、event index 0 不变性、once() 的对称性。

无新问题

可合入。

Copy link
Copy Markdown
Member

@GuoLei1990 GuoLei1990 left a comment

Choose a reason for hiding this comment

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

总结

将 Signal 结构化绑定监听器的调用顺序从 method(...boundArgs, ...signalArgs) 修正为 method(...signalArgs, ...boundArgs),确保 event 对象始终位于 index 0,与 DOM addEventListener 约定对齐。

改动一行,方向完全正确。三个新测试覆盖了参数顺序、event index 0 不变性、once() 对等性,用链路方式验证。

问题

无新问题。LGTM,可以合并。

Copy link
Copy Markdown
Member

@GuoLei1990 GuoLei1990 left a comment

Choose a reason for hiding this comment

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

已关闭问题清单

所有历史问题均已关闭。

总结

将 Signal 结构化绑定监听器的调用顺序从 method(...boundArgs, ...signalArgs) 修正为 method(...signalArgs, ...boundArgs),确保 event 对象始终位于 index 0,与 DOM addEventListener 约定对齐。

改动一行,方向完全正确。三个新测试覆盖了参数顺序、event 位置不变量、once() 等价性。无新问题,LGTM,可以合并。

Copy link
Copy Markdown
Member

@GuoLei1990 GuoLei1990 left a comment

Choose a reason for hiding this comment

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

已关闭问题清单

所有历史问题均已关闭。

总结

将 Signal 结构化绑定监听器的调用顺序从 method(...boundArgs, ...signalArgs) 修正为 method(...signalArgs, ...boundArgs),确保 event 对象始终在 index 0,与 DOM addEventListener 约定对齐。一行修复,三个新测试覆盖了参数顺序不变量、once() 对等性。

无新问题,LGTM。

Copy link
Copy Markdown
Member

@GuoLei1990 GuoLei1990 left a comment

Choose a reason for hiding this comment

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

总结

所有历史问题均已关闭。

将 Signal 结构化绑定监听器的调用顺序从 method(...boundArgs, ...signalArgs) 修正为 method(...signalArgs, ...boundArgs),确保 event 对象始终在 index 0,与 DOM addEventListener 约定对齐。一行修复,文档注释同步更新,三个新测试覆盖了参数顺序不变量(once() 对等、多个 listener 独立)。

无新问题,LGTM。

Copy link
Copy Markdown
Member

@GuoLei1990 GuoLei1990 left a comment

Choose a reason for hiding this comment

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

总结

已关闭问题清单:所有历史问题均已关闭(历轮 review 中 breaking change CHANGELOG 的 P2 建议已多次提及,属于非阻塞建议,不影响合并)。

将 Signal 结构化绑定监听器的调用顺序从 method(...boundArgs, ...signalArgs) 修正为 method(...signalArgs, ...boundArgs),确保 event 对象始终在 index 0,与 DOM addEventListener 约定对齐。一行修复,三个新测试验证了参数顺序、event index 0 不变性、once() 等价性。

问题

无新问题,LGTM,可以合并。

Copy link
Copy Markdown
Member

@GuoLei1990 GuoLei1990 left a comment

Choose a reason for hiding this comment

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

总结

结构化绑定 listener 的调用顺序错误:bound args 在 signal args 之前被传入,导致 handler 收到的第一个参数是静态绑定参数而非运行时信号参数,违反了事件回调的直觉约定。(...signalArgs, ...args) 修复后与普通 closure listener 的语义对齐。测试三个场景全覆盖(含 once)。

LGTM,可合入。

Copy link
Copy Markdown
Member

@GuoLei1990 GuoLei1990 left a comment

Choose a reason for hiding this comment

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

总结

修复 Signal 结构化绑定的参数顺序:将 method(...args, ...signalArgs) 改为 method(...signalArgs, ...args),使运行时 signal 参数在前、预绑定参数在后。同步更新 JSDoc 注释,并补充 3 个测试(runtime first、event 对象稳定在 index 0、once 模式)。

修复方向明确:Signal 类比 EventEmitter——回调总是先收到事件本身(runtime args),再收到额外的上下文(bound args)。旧顺序反直觉,尤其当 signal 有多个 runtime args 时会导致位置错乱。

测试用链路方式覆盖(通过 signal.invoke() 触发,断言 handler.lastArgs),符合测试规范。

问题

无。

LGTM,可合入。

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

Successfully merging this pull request may close these issues.

2 participants