Skip to content

fix(hooks): forward all arguments in useAppEvent for custom events - #341

Open
JonasPfi wants to merge 1 commit into
playcanvas:mainfrom
JonasPfi:fix/use-app-event-custom-args
Open

fix(hooks): forward all arguments in useAppEvent for custom events#341
JonasPfi wants to merge 1 commit into
playcanvas:mainfrom
JonasPfi:fix/use-app-event-custom-args

Conversation

@JonasPfi

Copy link
Copy Markdown

Summary

useAppEvent only forwarded the callback argument for the built-in update event. For every other event (including custom events fired via app.fire(...)) the callback was invoked with no arguments at all.

Root cause

const handler = useCallback(
    (...args: unknown[]) => {
        if (event === 'update') {
            (callback as (dt: number) => void)(args[0] as number);
        } else {
            (callback as () => void)(); // args dropped here
        }
    },
    [callback, event]
);

Only 'update' was special-cased to receive its argument. Every other event was invoked with zero arguments.

Fix

const handler = useCallback(
    (...args: unknown[]) => {
        (callback as (...args: unknown[]) => void)(...args);
    },
    [callback]
);

Forwards all arguments passed to app.fire(...) generically. update, prerender, and postrender continue to work exactly as before, since they simply receive their normal arguments through the same spread.

Testing

Added a new test (should forward all arguments to the callback for custom events) that fires a custom event via app.fire('levelComplete', 3, 1000) and asserts the callback receives both arguments.

I verified this test fails against the old implementation (callback called with no arguments) and passes against the fix, confirming the test actually exercises the bug rather than passing coincidentally.

Note: the existing test file has a comment explaining that built-in input events can't be fired in the null device type used in tests. This doesn't apply here : app.fire(...) is the underlying EventHandler mechanism, not a hardware input source, so it works fine in the test environment.

@changeset-bot

changeset-bot Bot commented Aug 16, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 2ed0d6f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@playcanvas/react Patch
@playcanvas/blocks Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@pkg-pr-new

pkg-pr-new Bot commented Sep 7, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@playcanvas/blocks@341
npm i https://pkg.pr.new/@playcanvas/react@341

commit: 2ed0d6f

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The behavioral fix is correct and low-risk, and the added regression test covers the reported failure mode.

Pull request overview

Fixes useAppEvent so event callbacks receive all arguments for non-built-in/custom events (including events fired via app.fire(...)), instead of silently dropping them.

Changes:

  • Simplified the internal handler in useAppEvent to forward (...args) directly to the provided callback.
  • Added a test that fires a custom event with multiple arguments and asserts the callback receives them.
  • Added a changeset to publish the fix as a patch release.
File summaries
File Description
packages/lib/src/hooks/use-app-event.ts Removes the update-only special casing and forwards all event arguments to the callback.
packages/lib/src/hooks/use-app-event.test.tsx Adds a regression test ensuring custom event arguments are forwarded through useAppEvent.
.changeset/deep-towns-attend.md Declares a patch release for the argument-forwarding fix.
Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +138 to +142
capturedApp!.fire('levelComplete', 3, 1000);

const [level, score] = levelCompleteCallback.mock.calls[0];
expect(level).toBe(3);
expect(score).toBe(1000);
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